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

Executors fail to recognize observable when function is not typehint #2449

Open
natestemen opened this issue Jul 17, 2024 · 3 comments
Open
Assignees
Labels
bug Something isn't working rem Readout Error Mitigation technique
Milestone

Comments

@natestemen
Copy link
Member

The following snippet only works when the execute function has a typehint for the functions return type.

import networkx as nx

from mitiq import MeasurementResult, benchmarks, rem
from mitiq.observable.observable import Observable
from mitiq.observable.pauli import PauliString

depth, num_qubits = 10, 4
circuit, bs = benchmarks.generate_mirror_circuit(
    nlayers=depth,
    two_qubit_gate_prob=1.0,
    connectivity_graph=nx.complete_graph(num_qubits),
    return_type="qiskit",
)


def execute(circuit, shots=100) -> MeasurementResult:
    fake_bitstrings = [[0, 0, 1, 1] for _ in range(shots)]

    return MeasurementResult(fake_bitstrings)


p_flip = 0.05
icm = rem.generate_inverse_confusion_matrix(len(bs), p_flip, p_flip)
obs = Observable(PauliString("ZZZZ"))

rem_value = rem.execute_with_rem(
    circuit,
    execute,
    obs,
    inverse_confusion_matrix=icm,
)

print(f"Expectation Value After REM: {rem_value}")

The error that is raised when removing the typehint is

ValueError: Executor and observable are not compatible. Executors
                returning expectation values as float must be used with
                observable=None

which comes from the Executor class. In particular a check to ensure the observable and return type of the executor are compatible.

elif (
observable is not None
and self._executor_return_type not in MeasurementResultLike
and self._executor_return_type not in DensityMatrixLike
):
raise ValueError(
"""Executor and observable are not compatible. Executors
returning expectation values as float must be used with
observable=None"""
)


Needless to say, this code should work regardless of the typehint. If investigation shows that the typehint truly is needed, then this requirement needs to be documented.

@natestemen natestemen added bug Something isn't working rem Readout Error Mitigation technique labels Jul 17, 2024
@bdg221 bdg221 self-assigned this Aug 28, 2024
@bdg221
Copy link
Collaborator

bdg221 commented Aug 28, 2024

@natestemen, I looked into this issue and it appears to be an issue when the Executor.evaluate function is called with an observable. This means that it applies to ZNE, REM, PEC, CDR, DDD, and QSE.

When a function is passed into any of the execute_with_XXX, eventually the function is used to create an Executor object. During that instantiation, the _executor_return_type is set.

self._executor_return_type = executor_annotation.get("return")

The executor_annotation.get call pulls from the typehint. If that is not set, then the _executor_return_type is None.

This comes back to bite us in Executor.evaluate when determining how to set all_circuits as this does the following check in the code:

if (observable is not None   and   self._executor_return_type in MeasurementResultLike)
...
elif(observable is not None   and    self._executor_return_type not in MeasurementResultLike
            and    self._executor_return_type not in DensityMatrixLike)
...
else

During this part of the code, it is assumed that if _executor_return_type is not in MeasurementResultLike and not in DensityMatrixLike it MUST be in FloatLike (as opposed to None).

If I correct the else if to be the following:

        elif (
            observable is not None
            and self._executor_return_type in FloatLike
        ):

Then not using the typehint would result in the else branch being taken and the following code being run:

            all_circuits = circuits
            result_step = 1

Let me know your thoughts, but I see the solution to be:

  • Update elif in Executor.evaluate to better handle not in MeasurementResultLike and DensityMatrixLike
  • Determine if the code in else is sufficient (looks like a MeasurementResultLike would be more optimized, but without the typehint it will still run)
  • This should be documented as either strongly recommended or required

@bdg221 bdg221 added this to the 0.40.0 milestone Aug 29, 2024
@natestemen natestemen changed the title REM functionality dependent on typehinting Executors fail to recognize observable when function is not typehint Sep 5, 2024
@natestemen
Copy link
Member Author

natestemen commented Sep 5, 2024

This means that it applies to ZNE, REM, PEC, CDR, DDD, and QSE.

Indeed, good catch. I've updated the issue title to reflect that fact.


After trying to write this comment for a full 2 days, I'm convinced their isn't a solution that satisfies everyone. There are two issues at play;

  1. This issue was raised after a group was surprised by the need for type hinting the executor.
  2. Raise error if executor and observable are not compatible #1140 arose when Mitiq was silently providing strange results when an observable was passed with an incompatible executor.

I'm convinced resolving both of these issues without making a 'test call' to the users Executor, is impossible. I do think there are some improvements we can make here both in documentation and code.

Documentation

These suggestions are independent of code changes. The warnings should highlight what the needs are for our API.

  1. We do mention in the The input function section of the Executors core concept page that type hinting or annotation is required. This should be upgraded to a warning1 and flushed out.
  2. The Using observables in error mitigation techniques section of the Observables core concept page should also have a warning and be flushed out.

Code

  1. The measure_in method of the Observable class should raise an error when the function results in more than one measurement on a single qubit. I would almost always assume this is unintended behavior. This will not completely fix the problem, however, as someone may add measurements inside their executor function.

  2. Modify the evaluate function of the Executor class to assume that if a user has passed an observable, then they are using a compatible Executor. I.e. remove this line, and the following elif block.

  3. We should ensure our tests have cases that cover the following cases, adding them where needed.

    mitigation method executor type type hinted observable
    1. ZNE MeasurementResult
    2. ZNE MeasurementResult
    3. ZNE float 2
    4. REM MeasurementResult
    5. REM MeasurementResult

Footnotes

  1. Details about admonitions can be found here: https://mystmd.org/guide/admonitions.

  2. This is likely already tested somewhere, just a matter of finding it.

@FarLab
Copy link
Contributor

FarLab commented Sep 5, 2024

I like this idea @natestemen, to assume the user has defined a compatible executor when passing an observable. If the executor is not compatible, the user gets an error down the road that should make it clear the executor is not compatible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working rem Readout Error Mitigation technique
Projects
None yet
Development

No branches or pull requests

3 participants