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

Measurement confusion maps #5480

Merged

Conversation

95-martin-orion
Copy link
Collaborator

@95-martin-orion 95-martin-orion commented Jun 10, 2022

Fixes #4348.

This adds an optional confusion_map field to MeasurementGate which probabilistically changes the results of that measurement without changing the state of the targeted qubits.

Compare with TensoredConfusionMatrices introduced in #4854. I opted to provide a conversion to that type instead of using it directly as it differs too greatly from what is needed here:

  • This needs an index map (MeasurementGate is qubit-agnostic), TCM uses qubit and confuser lists
  • This is for confusing results, TCM.apply "unconfuses" results
  • This is in cirq/ops, TCM is in cirq/experiemental

EDIT: It was pointed out that this potentially breaks external measurement and simulator behavior by requiring the confusion_map parameter. PR #5534 resolves this.

@95-martin-orion 95-martin-orion requested review from mrwojtek, a team, vtomole and cduck as code owners June 10, 2022 15:58
@CirqBot CirqBot added the size: L 250< lines changed <1000 label Jun 10, 2022
@daxfohl
Copy link
Contributor

daxfohl commented Jun 10, 2022

Can you add a note in DeferredMeasurementsTransformer docstring that measurement confusion maps are not accounted for, and create a github task for fixing that?

@95-martin-orion
Copy link
Collaborator Author

Can you add a note in DeferredMeasurementsTransformer docstring that measurement confusion maps are not accounted for, and create a github task for fixing that?

It now raises an error on confused measurements. Opened #5482 (marked after-1.0) for the eventual fix.

@95-martin-orion
Copy link
Collaborator Author

95-martin-orion commented Jun 14, 2022

Latest commit fixes an issue I ran into while documenting this: SimulatesSamples uses a different path to collect measurement results, and needed to implement confusion independently. Will add tests for the new code in an upcoming commit.

Copy link
Collaborator

@dabacon dabacon left a comment

Choose a reason for hiding this comment

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

LGTM

@95-martin-orion 95-martin-orion merged commit 2bff437 into quantumlib:master Jun 15, 2022
@95-martin-orion 95-martin-orion deleted the cirq-confused-measure branch June 15, 2022 20:57
Comment on lines +103 to +109
def measure(
self,
qubits: Sequence['cirq.Qid'],
key: str,
invert_mask: Sequence[bool],
confusion_map: Dict[Tuple[int, ...], np.ndarray],
):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Without a default value for confusion_map argument, wouldn't this break all user code that uses this public API ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. The only Cirq code that calls this is MeasurementGate._act_on_, but there's still some risk:

  1. External measurement gates don't pass this argument, and would break
  2. External simulators which reimplement this method won't expect this argument, and would break

(2) actually seems more likely to me than (1), but we can guard against both. I'll put up another PR and mark this one as breaking.

confusion_map: Dict[Tuple[int, ...], np.ndarray],
):
"""Applies confusion matrices to measured results."""
confused = list(bits)
Copy link
Collaborator

Choose a reason for hiding this comment

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

bits is already a List[int] so this looks redundant. Should bits be a Sequence[int] instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The purpose of this is not to convert to list but to copy bits.

@95-martin-orion 95-martin-orion added the BREAKING CHANGE For pull requests that are important to mention in release notes. label Jun 16, 2022
CirqBot pushed a commit that referenced this pull request Jun 16, 2022
Fixes the breakage created by #5480.
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
* Measurement confusion maps

* format

* mypy+format

* Error on deferred confused measure

* Also change SimulatesSamples behavior.

* Test SimulatesSamples

* docstring zero note
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
* Measurement confusion maps

* format

* mypy+format

* Error on deferred confused measure

* Also change SimulatesSamples behavior.

* Test SimulatesSamples

* docstring zero note
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BREAKING CHANGE For pull requests that are important to mention in release notes. size: L 250< lines changed <1000
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Noisy Measurement
5 participants