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

Add eigvals support to ExpectationMP, CountsMP, VarianceMP, SampleMP #5463

Merged
merged 23 commits into from
Apr 9, 2024

Conversation

astralcai
Copy link
Contributor

Context:
MeasurementProcess supports specifying eigvals instead of obs, which should include sample based measurements such as ExpectationMP, VarianceMP. However, process_samples of these measurements does not work if the measurement was created using eigvals

Description of the Change:
Adds eigvals support to variance sample measurements

Benefits:
Bugfix

Possible Drawbacks:
This fix only makes sure that the process_samples method would work. Specifically for expval, as it is also a state measurement, currently taking a expval in a qnode with only eigvals specified does not work.

Related GitHub Issues:
Fixes #5432
[sc-59519]

Copy link

codecov bot commented Apr 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (master@f10e98f). Click here to learn what that means.

Additional details and impacted files
@@            Coverage Diff            @@
##             master    #5463   +/-   ##
=========================================
  Coverage          ?   99.66%           
=========================================
  Files             ?      402           
  Lines             ?    37347           
  Branches          ?        0           
=========================================
  Hits              ?    37221           
  Misses            ?      126           
  Partials          ?        0           

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

@astralcai astralcai marked this pull request as ready for review April 3, 2024 14:10
Copy link
Contributor

@lillian542 lillian542 left a comment

Choose a reason for hiding this comment

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

Looking good! Left a bit of feedback regarding the wires check, otherwise just a couple very minor comments from me 🚀

pennylane/measurements/counts.py Outdated Show resolved Hide resolved
pennylane/measurements/expval.py Show resolved Hide resolved
pennylane/measurements/var.py Show resolved Hide resolved
Copy link
Contributor

@PietropaoloFrisoni PietropaoloFrisoni left a comment

Choose a reason for hiding this comment

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

Fantastic, thanks @astralcai. I notice that some statements in strings have periods, and others do not (and I am not a massive fan of capital words), but it seems to me you just adapted such a style to the existing context.

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.

I'm a little hesitant to make specifying eigenvalues user-facing via the constructor functions.

While we do have internal logic for having eigvals+wires as the description for a measurement (see MeasurementProcess.expand), it isn't particularly well supported right now (see PR #5445 for some other places things are going wrong). The bugfix is part of the first push to improving our support for this specification, but we need to more testing and support improvement.

I would recommend breaking this PR into two, one fixing the internal bug, and another making this representation user-facing. In that way, the bug fix won't be blocked by product concerns.

@astralcai
Copy link
Contributor Author

I'm a little hesitant to make specifying eigenvalues user-facing via the constructor functions.

While we do have internal logic for having eigvals+wires as the description for a measurement (see MeasurementProcess.expand), it isn't particularly well supported right now (see PR #5445 for some other places things are going wrong). The bugfix is part of the first push to improving our support for this specification, but we need to more testing and support improvement.

I would recommend breaking this PR into two, one fixing the internal bug, and another making this representation user-facing. In that way, the bug fix won't be blocked by product concerns.

ExpectationMP.process_samples calls qml.sample to create a SampleMP, and uses SampleMP.process_samples to preprocess the samples. Then it calculates the expectation value by taking the mean of the processed sample. The internal bug was caused by qml.sample not being able to handle eigvals. In order to fix the internal bug, eigvals must be added as an argument to qml.sample, but it can be left out of the other measurements for now. What do you think?

@albi3ro
Copy link
Contributor

albi3ro commented Apr 5, 2024

I'm a little hesitant to make specifying eigenvalues user-facing via the constructor functions.
While we do have internal logic for having eigvals+wires as the description for a measurement (see MeasurementProcess.expand), it isn't particularly well supported right now (see PR #5445 for some other places things are going wrong). The bugfix is part of the first push to improving our support for this specification, but we need to more testing and support improvement.
I would recommend breaking this PR into two, one fixing the internal bug, and another making this representation user-facing. In that way, the bug fix won't be blocked by product concerns.

ExpectationMP.process_samples calls qml.sample to create a SampleMP, and uses SampleMP.process_samples to preprocess the samples. Then it calculates the expectation value by taking the mean of the processed sample. The internal bug was caused by qml.sample not being able to handle eigvals. In order to fix the internal bug, eigvals must be added as an argument to qml.sample, but it can be left out of the other measurements for now. What do you think?

Could we just directly create a SampleMP?

@trbromley
Copy link
Contributor

+1 to @albi3ro's comment, do we need to add another argument to qml.sample(), etc., to fix this bug?

@astralcai
Copy link
Contributor Author

qml.sample contains logic that handles the creation of a SampleMP. I can create a helper method to contain that logic. I will make the update.

@albi3ro
Copy link
Contributor

albi3ro commented Apr 5, 2024

qml.sample contains logic that handles the creation of a SampleMP. I can create a helper method to contain that logic. I will make the update.

I'd be fine with moving more validation and preprocessing into SampleMP.__init__.

@astralcai astralcai changed the title Add eigvals support to expval, counts, var, sample Add eigvals support to ExpectationMP, CountsMP, VarianceMP, SampleMP Apr 8, 2024
@astralcai astralcai requested a review from albi3ro April 8, 2024 17:19
astralcai and others added 2 commits April 8, 2024 16:32
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.

Thanks for adding this! Looks great :)

@astralcai astralcai enabled auto-merge (squash) April 9, 2024 20:59
@astralcai astralcai merged commit 1089bb3 into master Apr 9, 2024
38 checks passed
@astralcai astralcai deleted the eigvals-meas branch April 9, 2024 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Issues with ExpectationMP.process_samples
5 participants