Address executor and observable incompatibility #2514

bdg221 · 2024-09-25T23:00:49Z

Description

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.

I added unit tests for new code.
I used type hints in function signatures.
I used Google-style docstrings for functions.
I updated the documentation where relevant.

bdg221 · 2024-09-25T23:25:53Z

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 · 2024-09-26T18:43:29Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.73%. Comparing base (6f912f6) to head (8a68dfe).
Report is 15 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2514      +/-   ##
==========================================
+ Coverage   98.71%   98.73%   +0.01%     
==========================================
  Files          89       92       +3     
  Lines        4131     4185      +54     
==========================================
+ Hits         4078     4132      +54     
  Misses         53       53

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

bdg221 · 2024-09-27T22:22:02Z

@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

mitiq/executor/executor.py

bdg221 · 2024-10-04T17:47:05Z

@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.

natestemen

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 :)

natestemen · 2024-10-28T21:59:04Z

mitiq/executor/executor.py

+    np.float32,
+    np.float64,


Were these needed when adding further tests?

I found the need to add these when running make test. Here is the result without those in FloatLike:

And with those 2 lines, I don't see the errors:

@natestemen do you see this behavior too?

I remembered why these two are in FloatLike. Since we are trying to assume things if a return type is not specified, we check the type of what is returned from self.run(). If we do not include them, then the ifs during Parse the results. will fail, especially if we no longer look at self._executor_return_type as mentioned #2514 (comment)

mitiq/observable/pauli.py

mitiq/executor/tests/test_executor.py

natestemen · 2024-10-28T22:28:26Z

mitiq/executor/tests/test_executor.py

-def test_executor_observable_compatibility_check(execute, obs):
-    q = cirq.LineQubit(0)
-    circuits = [cirq.Circuit(cirq.X(q)), cirq.Circuit(cirq.H(q), cirq.Z(q))]
-
-    executor = Executor(execute)
-
-    with pytest.raises(ValueError, match="are not compatible"):
-        executor.evaluate(circuits, obs)


Does this test need to be removed? I think I'm missing something because I don't quite see why these are not compatible.

All of those executors return floats or list[floats]. We do not allow you to have an observables with an executor that is returning FloatLike. However, this was an old test from years ago. This is now covered in def test_executor_float_with_observable_typed().

natestemen · 2024-10-28T22:31:55Z

mitiq/executor/executor.py

+        elif observable is None:
+            # Type hinted as DensityMatrixLik but no observable is set
+            if self._executor_return_type in DensityMatrixLike:
+                raise ValueError(
+                    "When using a density matrix like result, an observable "
+                    "is required."
+                )
+            # Type hinted as MeasurementResulteLike but no observable is set
+            elif self._executor_return_type in MeasurementResultLike:
+                raise ValueError(
+                    "When using a measurement, or bitstring, like result, an "
+                    "observable is required."
+                )


These checks make it impossible to use an Executor to run and return density matrices and counts. I don't think we want to limit users in that way, do we?

It does make it impossible to use an Executor to run and return density matrices and counts if an observable is not provided. This limitation actually comes from parsing the results where observable is used by density matrices _expectation_from_density_matrix() and counts _expectation_from_measurements().

natestemen · 2024-10-28T22:35:36Z

mitiq/executor/executor.py

-        all_results = self.run(all_circuits, force_run_all, **kwargs)
+        try:
+            all_results = self.run(all_circuits, force_run_all, **kwargs)
+        except Exception:


Catch specific exceptions when possible. What fails in the above?

This exception is coming from the backend which makes it tough to be more specific. This is being used to capture the scenario of a MeasurementResultLike expected return type, but without one being specified. In that situation, an observable should be passed in but we didn't go through the if at line 178 to add the measurements to the circuit(s). Currently, the different backends would throw an error like:

Qiskit: QiskitError: 'No counts for experiment "0"'
Cirq: ValueError: Circuit has no measurements to sample.

The cost of re-running an experiment may not be worth it for this scenario. Of all the times the executors fail, how frequently do we with think this is the scenario and how costly is it to re-run in those other scenarios?

natestemen · 2024-10-28T22:35:49Z

mitiq/executor/executor.py

+                ]
+                all_results = self.run(all_circuits, force_run_all, **kwargs)
+            else:
+                raise


What happens here?

This is checking if the observable is not none and trying to see if the result type was meant to be MeasurementResultLike. If the return type was specified then the measurements would have been added to the circuit(s).

natestemen · 2024-10-28T22:37:02Z

mitiq/executor/executor.py

+        manual_return_type = None
+        if len(all_results) > 0:
+            manual_return_type = type(all_results[0])


Should we even care about the _executor_return_type at this point?

If an executor returns something different than what is specified there will be other errors thrown. You are right, at this point we can stick with the manual_return_type.

Fixed in 3c7b027

Just another note on this. The type call will only return the high level object type like list without the element types. FloatLike, MeasurementResultLike, and DensityMatrixLike are expecting List[float] for example since this was previously coming from the _executor_return_type.

I handle this with 7c4a60f by checking for if the result falls under a Sequence or Iterable and if so, I save the first element type to the manual_return_type.

Should I add more unit testing since I just introduced this if branching?

EDIT: Too late. I add test and updated the check to see if the Sequence is empty in 8a68dfe

Co-authored-by: nate stemen <[email protected]>

FarLab · 2024-11-15T11:58:20Z

mitiq/executor/executor.py

        # Run all required circuits.
-        all_results = self.run(all_circuits, force_run_all, **kwargs)
+        try:
+            all_results = self.run(all_circuits, force_run_all, **kwargs)
+        except Exception:
+            if observable is not None and self._executor_return_type is None:
+                all_circuits = [
+                    circuit_with_measurements
+                    for circuit in circuits
+                    for circuit_with_measurements in observable.measure_in(
+                        circuit
+                    )
+                ]
+                all_results = self.run(all_circuits, force_run_all, **kwargs)
+            else:
+                raise


I like the idea of just executing one of the circuits and checking the return type. Does this mean we can move all the compatibility checks in lines 156-175 after this manual checking of return type? This would remove the need of _executor_return_type variable and no typehinting is needed anymore. Maybe I am missing something.

A couple of things.

This is NOT checking the return type of a single circuit, but instead this is running all circuits with the executor.

The checks in lines 156-175 are specifically checking the following cases based off limitations already in the execute function.

FloatLike - expected the circuits to already include measurements and there should NOT be an observable. The executor does not handle observables for FloatLike results.

DensityMatrixLike and MeasurementLike - MeasurementResultLike handles observables in lines 182-187 and both respectively use OBSERVABLE._expectation_from_density_matrix() and OBSERVABLE._expectation_from_measurements()

Since these are requirements, I would think it would be better to check and break out in bad scenarios, instead of trying to run the circuits and then error out.

Ah yes, you are right that it is running all circuits! I guess I was looking at lines 209-216 where you use the manual return type (of the first circuit?) to parse results. I was thinking to just run the first circuit to get the manual return type and do all the checks on observable compatibility afterwards. This is a completely different approach than using _executor_return_type, so understandable if we don't want to that.

The main use of _executor_return_type (outside of what I have done in this PR) is to check if it is MeasurementResultLike and there is an observable so that observable.measure_in() can be called to add measurements into the circuit. Without doing this, the backends complain. I can't think of a way to do this without the _executor_return_type since calling run() to try to get back a manual return type throws errors.

FarLab

It looks good to me!

bdg221 added 5 commits September 24, 2024 15:14

Remove check for observer return type

a6e09f6

Add error for more than one measurement per qubit

1a42969

Remove commented line

aa2597d

Add test for multiple measurements on a qubit

a5ec9d0

Update executor and observable documentation

a08a87f

bdg221 requested a review from natestemen September 25, 2024 23:00

bdg221 self-assigned this Sep 25, 2024

bdg221 added 4 commits September 25, 2024 17:14

Handle correct case without measurements

edcf03d

Add back line that broke density matrices

4225339

Remove test for condition that was removed

6a6fa13

Remove no longer required test parameters

1d447cf

bdg221 added 2 commits September 27, 2024 10:35

executor_observable compatability with typehint and tests

163b045

Add logic to check executor observable compat and tests

2def914

bdg221 requested a review from cosenal September 27, 2024 22:22

bdg221 commented Sep 27, 2024

View reviewed changes

mitiq/executor/executor.py Outdated Show resolved Hide resolved

bdg221 added 2 commits October 4, 2024 10:29

retry failed attempt with measurement and check returned type

7c76316

add test to compare typed vs nontyped

72c0168

Add numpy float64 to FloatLike

dc32389

natestemen requested changes Oct 28, 2024

View reviewed changes

bdg221 and others added 6 commits October 29, 2024 15:49

remove uncessary assert

1333ebf

update f-string formatting

99c7374

Co-authored-by: nate stemen <[email protected]>

Add back executor call in float test

bbd1298

Update finding and checking existing qubit with measurements

02bd3f5

Parse results using manual return type

3c7b027

Check type inside Sequence and Iterators

7c4a60f

bdg221 requested a review from natestemen October 30, 2024 20:51

bdg221 added 2 commits October 30, 2024 14:50

Add a second qubit to multi measurement test

a0e4cda

Update Sequence check and add tests

8a68dfe

purva-thakre requested a review from FarLab November 8, 2024 17:08

FarLab reviewed Nov 15, 2024

View reviewed changes

bdg221 requested a review from FarLab November 19, 2024 03:31

FarLab approved these changes Nov 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address executor and observable incompatibility #2514

Address executor and observable incompatibility #2514

bdg221 commented Sep 25, 2024 •

edited

Loading

bdg221 commented Sep 25, 2024

codecov bot commented Sep 26, 2024 •

edited

Loading

bdg221 commented Sep 27, 2024 •

edited

Loading

bdg221 commented Oct 4, 2024 •

edited

Loading

natestemen left a comment

natestemen Oct 28, 2024

bdg221 Oct 29, 2024

bdg221 Oct 30, 2024 •

edited

Loading

natestemen Oct 28, 2024

bdg221 Oct 30, 2024 •

edited

Loading

natestemen Oct 28, 2024

bdg221 Oct 29, 2024

natestemen Oct 28, 2024

bdg221 Oct 29, 2024 •

edited

Loading

natestemen Oct 28, 2024

bdg221 Oct 29, 2024

natestemen Oct 28, 2024

bdg221 Oct 30, 2024

bdg221 Oct 30, 2024 •

edited

Loading

bdg221 Oct 30, 2024

bdg221 Oct 30, 2024 •

edited

Loading

FarLab Nov 15, 2024

bdg221 Nov 15, 2024

FarLab Nov 15, 2024

bdg221 Nov 15, 2024

FarLab left a comment

Address executor and observable incompatibility #2514

Are you sure you want to change the base?

Address executor and observable incompatibility #2514

Conversation

bdg221 commented Sep 25, 2024 • edited Loading

Description

License

bdg221 commented Sep 25, 2024

codecov bot commented Sep 26, 2024 • edited Loading

Codecov Report

bdg221 commented Sep 27, 2024 • edited Loading

bdg221 commented Oct 4, 2024 • edited Loading

natestemen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdg221 Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdg221 Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdg221 Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdg221 Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdg221 Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FarLab left a comment

Choose a reason for hiding this comment

bdg221 commented Sep 25, 2024 •

edited

Loading

codecov bot commented Sep 26, 2024 •

edited

Loading

bdg221 commented Sep 27, 2024 •

edited

Loading

bdg221 commented Oct 4, 2024 •

edited

Loading

bdg221 Oct 30, 2024 •

edited

Loading

bdg221 Oct 30, 2024 •

edited

Loading

bdg221 Oct 29, 2024 •

edited

Loading

bdg221 Oct 30, 2024 •

edited

Loading

bdg221 Oct 30, 2024 •

edited

Loading