-
-
Notifications
You must be signed in to change notification settings - Fork 158
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
base: main
Are you sure you want to change the base?
Conversation
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. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2514 +/- ##
==========================================
- Coverage 98.71% 98.70% -0.01%
==========================================
Files 89 90 +1
Lines 4131 4178 +47
==========================================
+ Hits 4078 4124 +46
- Misses 53 54 +1 ☔ View full report in Codecov by Sentry. |
@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: |
mitiq/executor/executor.py
Outdated
) | ||
== 0 | ||
): | ||
if self._executor_return_type is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is line 192 to 204 that is causing the tests to fail.
@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 in the following scenarios:
When the return type is After calling 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. |
Description
Fixes #2449
This PR address code changes in the
Executor
andPauli
classes to account for when anObservable
is passed to an executor along with a circuit that already has a measurement on a qubit effected by theObservable
.Also, I removed a check in the
Executor
class, with the assumption the user is passing a validexecutor
if anobservable
is used.Furthermore, the docs for the
Executor
andObservable
have been updated with the annotation/typehinting requirement.Finally, additional unit testing has been added to ensure that incompatible
Executor
s andObservable
s are caught and an appropriate message is given to the user.License
Before opening the PR, please ensure you have completed the following where appropriate.