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

Address executor and observable incompatibility #2514

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

bdg221
Copy link
Collaborator

@bdg221 bdg221 commented Sep 25, 2024

Description

Fixes #2449

This PR address code changes in the Executor and Pauli classes to account for when an Observable is passed to an executor along with a circuit that already has a measurement on a qubit effected by the Observable.

Also, I removed a check in the Executor class, with the assumption the user is passing a valid executor if an observable is used.

Furthermore, the docs for the Executor and Observable have been updated with the annotation/typehinting requirement.

Finally, additional unit testing has been added to ensure that incompatible Executors and Observables are caught and an appropriate message is given to the user.


License

  • I license this contribution under the terms of the GNU GPL, version 3 and grant Unitary Fund the right to provide additional permissions as described in section 7 of the GNU GPL, version 3.

Before opening the PR, please ensure you have completed the following where appropriate.

@bdg221 bdg221 requested a review from natestemen September 25, 2024 23:00
@bdg221 bdg221 self-assigned this Sep 25, 2024
@bdg221
Copy link
Collaborator Author

bdg221 commented Sep 25, 2024

I believe there are additional unit tests that need to be created to ensure that we cover as many scenarios as possible. This is especially true when we assume the user has defined a compatible executor when passing an observable.

I plan to discuss this in more detail with @natestemen to ensure appropriate test coverage.

Copy link

codecov bot commented Sep 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.70%. Comparing base (7648648) to head (b78e824).
Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2514      +/-   ##
==========================================
- Coverage   98.73%   98.70%   -0.03%     
==========================================
  Files          92       92              
  Lines        4174     4183       +9     
==========================================
+ Hits         4121     4129       +8     
- Misses         53       54       +1     

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

@bdg221
Copy link
Collaborator Author

bdg221 commented Sep 27, 2024

@natestemen @cosenal I ran into a bit of an issue and I was curious how you think I should proceed. I added logic into the executor that will check the scenario where the executor does not have a return type specified and an observable is not passed. In this scenario we assume that the return type is float, therefore I am checking the circuits to make sure that there is a measurement included, if not I check if the return type was passed in (error messaging telling the user to include measurements in their circuit since they meant for it to return a float) or not (error message saying to use type hinting and if meant to return a float then add measurements.)

I believe this is what we want, however I found that we have over 200 tests that fail when the code is implemented. The executors in the tests that fail do not use type hinting and also do not include measurement gates (since typically a simple float is return like the length of the circuit or a random number.)

I can certainly go through and clean those up, however I am wondering if I should break that out to a new issue/PR because it is a bit involved.

We could also let the error come from the backend, however those messages may vary. We also cannot mention using type hinting with this option. Examples of some error messages from cirq and qiskit:
Circuit has no measurements to sample.
No counts for experiment

@bdg221 bdg221 requested a review from cosenal September 27, 2024 22:22
mitiq/executor/executor.py Outdated Show resolved Hide resolved
@bdg221
Copy link
Collaborator Author

bdg221 commented Oct 4, 2024

@natestemen @cosenal With this latest update, I am only checking now for obvious incompatibility. This means that if a user does use type hinting and does/does not include an observable it will throw errors if the following scenarios are not met:

FloatLike should NOT have an observable
DensityMatrixLike MUST have an observable
MeasurementResultLike MUST have an observable

When the return type is MeasurementResultLike, we use the observable to add measurement gates to the circuit before call self.run(). Previously, if the return type was not specified this would result in an error from the backend because the circuit would not have measurements. However, now I added a test if self.run() fails to see if there is an observable and if so to add the measurement(s) to the circuit(s) and try self.run() again.

After calling self.run() successfully, we normally parse the results. FloatLike grabs np.real_if_close(), DensityMatrixLike uses the observable._expectation_from_density_matrix(), and MeasurementResultLike uses the observable._expectation_from_measurements(). While previously the code was looking self._executor_return_type, I am also now using the return type of the first object in the sequence returned by self.run().

I added tests that check the obviously bad scenarios when type hinting is used. Also, I compare the results of typed versus non-typed to make that if we assume the user does things correctly without type hinting that they get the correct answer.

Copy link
Member

@natestemen natestemen 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 getting started with this Brian!

Apologies on the long delay for the review. In general since the Executor is so core to a lot of things in Mitiq, I'm hesitant to make changes when we don't understand the side effects thoroughly. Hopefully we can mitigate some of those worries through good tests and some back and forth :)

mitiq/executor/executor.py Outdated Show resolved Hide resolved
mitiq/observable/pauli.py Outdated Show resolved Hide resolved
mitiq/observable/pauli.py Outdated Show resolved Hide resolved
mitiq/executor/tests/test_executor.py Outdated Show resolved Hide resolved
mitiq/executor/tests/test_executor.py Show resolved Hide resolved
mitiq/executor/executor.py Outdated Show resolved Hide resolved
mitiq/executor/executor.py Outdated Show resolved Hide resolved
mitiq/executor/executor.py Outdated Show resolved Hide resolved
mitiq/executor/executor.py Outdated Show resolved Hide resolved
@bdg221 bdg221 requested a review from natestemen October 30, 2024 20:51
@purva-thakre purva-thakre requested a review from FarLab November 8, 2024 17:08
mitiq/executor/executor.py Outdated Show resolved Hide resolved
@bdg221 bdg221 requested a review from FarLab November 19, 2024 03:31
Copy link
Contributor

@FarLab FarLab left a comment

Choose a reason for hiding this comment

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

It looks good to me!

Copy link
Member

@natestemen natestemen 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 the continued effort on this Brian! I know we all want to merge this, but as it stands there are a few blockers (like running more circuits and a user might have anticipated).

Independent of the specifics of this PR, your work has been incredibly valuable in helping me understand the difficulties surrounding the Executor.

mitiq/executor/executor.py Outdated Show resolved Hide resolved
mitiq/executor/executor.py Outdated Show resolved Hide resolved
mitiq/executor/executor.py Outdated Show resolved Hide resolved
@bdg221
Copy link
Collaborator Author

bdg221 commented Dec 13, 2024

At this point, this PR is checking for explicit incompatibility between the executors and observables. It provides clear error messages and includes additional unit tests.

@bdg221 bdg221 requested a review from natestemen December 13, 2024 18:04
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.

Executors fail to recognize observable when function is not typehint
3 participants