Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement process_counts in ExpectationMP, VarianceMP and CountsMP #5256

Merged
merged 16 commits into from
Mar 12, 2024

Conversation

Tarun-Kumar07
Copy link
Contributor

@Tarun-Kumar07 Tarun-Kumar07 commented Feb 25, 2024

Context:
Under #4941 abstract method process_counts was added to SampleMeasurement. This method provides support to process counts dictionary produced by external systems.

Description of the Change:

  • Implement process_counts in ExpectationMP, VarianceMP and CountsMP.
  • While implementing process_counts in CountsMP some common logic was extracted from ProbabilityMP to CountsMP

Benefits:
The below methods won't throw NotImplementedError exception

  • ExpectationMP.process_counts
  • VarianceMP.process_counts
  • CountsMP.process_counts

Possible Drawbacks:

  • All classes which inherit from SampleMeasurement implement process_counts except SampleMP.
    Should I add an implementation for that ?
  • After implementing process_counts in all child classes can we make the method abstract similar to process_samples ? This might break some tests where some classes inherit from SampleMeasurement as there process_counts is not implemented
  • It is assumed that counts dictionary is valid and caller is responsible to call with valid dictionary. It has already been discussed in this conversation and this is a design choice.

Related GitHub Issues:
This PR


Further details
I'm excited to be making my first open-source contribution with this PR 😄 .
As a newcomer to the community, I'm eager to learn and improve.
Any feedback for enhancements would be greatly appreciated!

Internal Shortcut Stories

Copy link

codecov bot commented Feb 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.63%. Comparing base (9d54b1c) to head (92fc70f).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5256      +/-   ##
==========================================
- Coverage   99.64%   99.63%   -0.01%     
==========================================
  Files         401      401              
  Lines       37215    36963     -252     
==========================================
- Hits        37082    36829     -253     
- Misses        133      134       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Renaming counts argument doesn't help new pylint error `arguments-renamed` occurs.
@Tarun-Kumar07
Copy link
Contributor Author

I am not sure how to fix failing CI check, Can the reviewers please guide me?

@trbromley
Copy link
Contributor

Thanks a lot for opening this @Tarun-Kumar07! We'll give this a review soon and also help with the CI issue you mentioned. Sorry if things are a little delayed this week, we're busy preparing for the PennyLane 0.35 release next week 🚀

@Tarun-Kumar07
Copy link
Contributor Author

@trbromley Thank you for your response.
If there's anything I can do to help with the PennyLane 0.35 release preparations, please let me know.
I'd be more than happy to assist in any way I can! 🚀

@timmysilv
Copy link
Contributor

hi @Tarun-Kumar07 , thanks for bearing with us! I've pushed a fix for the strange "Documentation Check" issue to the master branch. If you update your fork with the latest master, it should disappear 🤞 give it a try, I'll keep an eye out to make sure it's resolved here.

@Tarun-Kumar07
Copy link
Contributor Author

@timmysilv all CI checks passed 🎉 , thanks for the fix.
This PR is now ready for review.

@Tarun-Kumar07 Tarun-Kumar07 changed the title Implement process counts in ExpectationMP, VarianceMP and CountsMP Implement process_counts in ExpectationMP, VarianceMP and CountsMP Mar 5, 2024
Copy link
Contributor

@albi3ro albi3ro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about the delay in review.

This is looking really great @Tarun-Kumar07 :)

I only have two main requests:

  1. stripping out zeros entries if all_outcomes=False
  2. Using qml.QueuingManager.stop_recording() around any location that creates an intermediary measurement process

But once those are done, I'll be happy to approve.

pennylane/measurements/counts.py Show resolved Hide resolved
pennylane/measurements/expval.py Outdated Show resolved Hide resolved
pennylane/measurements/probs.py Outdated Show resolved Hide resolved
pennylane/measurements/var.py Outdated Show resolved Hide resolved
@Tarun-Kumar07
Copy link
Contributor Author

Hey @albi3ro,

I noticed that all classes which inherit from SampleMeasurement implement process_counts except SampleMP. Should I add an implementation for that?

@Tarun-Kumar07 Tarun-Kumar07 force-pushed the implement-process_counts branch from db6f2d5 to dada914 Compare March 11, 2024 03:04
@Tarun-Kumar07 Tarun-Kumar07 force-pushed the implement-process_counts branch from dada914 to fb5f96e Compare March 11, 2024 03:11
@Tarun-Kumar07 Tarun-Kumar07 requested a review from albi3ro March 11, 2024 03:48
Copy link
Contributor

@albi3ro albi3ro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just left a request to change the changelog entry to reference this PR, rather than the issues.

Looking great to me 🎉

Yes, we could also use SampleMP.process_counts, but we could potentially stick that into an independent PR instead.

@Tarun-Kumar07
Copy link
Contributor Author

Hi @albi3ro and @trbromley,

Thanks for reviewing my PR.
It's my first open-source contribution, and I'm eager to contribute even more 🎉.

Appreciate it!

@Tarun-Kumar07
Copy link
Contributor Author

@albi3ro, regarding adding SampleMP.process_counts.
Should I create a PR directly or make an issue about it?
What's the preferred process?

@Tarun-Kumar07 Tarun-Kumar07 requested a review from trbromley March 12, 2024 03:46
@trbromley trbromley requested a review from a team March 12, 2024 13:44
Copy link
Contributor

@astralcai astralcai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks Good! 🎸

@albi3ro
Copy link
Contributor

albi3ro commented Mar 12, 2024

@albi3ro, regarding adding SampleMP.process_counts. Should I create a PR directly or make an issue about it? What's the preferred process?

I'll go ahead and create an issue right now.

Since this PR now has two approvals 🎉 , I'll go ahead and enable auto-merge. Congrats on the PR!

@albi3ro albi3ro merged commit 5157192 into PennyLaneAI:master Mar 12, 2024
40 checks passed
@trbromley
Copy link
Contributor

Thanks @Tarun-Kumar07! Great job with this PR.

We often tag contributors in our release marketing on Twitter/X (this will happen at the start of May). If you would optionally like to be tagged, please could you share your username?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants