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

Inconsistent shape of Result.records #5000

Closed
95-martin-orion opened this issue Feb 16, 2022 · 2 comments · Fixed by #5014
Closed

Inconsistent shape of Result.records #5000

95-martin-orion opened this issue Feb 16, 2022 · 2 comments · Fixed by #5014
Assignees
Labels
area/measurements kind/bug-report Something doesn't seem to work. triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on

Comments

@95-martin-orion
Copy link
Collaborator

As seen in the qsim nightly compatibility runs. CC @daxfohl.

These errors seem to indicate that the values in Result._records sometimes unpack to two-element values. It's possible that this is a result of interaction between qsimcirq and the new Result behavior, but if so that indicates backwards-incompatibility that needs to be addressed.

@95-martin-orion 95-martin-orion added the kind/bug-report Something doesn't seem to work. label Feb 16, 2022
@daxfohl
Copy link
Collaborator

daxfohl commented Feb 16, 2022

This actually occurred to me last night, that something like this might happen.

So in simulator.py, we changed the pydoc around abstract method SimulatesSamples._run to say it's a 3D array, updated SimulatorBase._run to do so, and back in SimulatesSamples.run_sweep_iter we expect a 3D response. I had overlooked the fact that external simulators might implement SimulatesSamples but not SimulatorBase. (Nothing in cirq proper does).

My initial thought is that the best approach would be to update the ResultDict initializer such that, instead of separate measurements and records args, instead there's a single data argument that can be either 2D or 3D, and ResultDict checks data.ndim and populates the corresponding member variable.

Do you mind taking it since I don't have any recent experience with qsim? (Also maybe create another test like simulator_test.py::test_async_sample except have _run return a 2D array and verify that it succeeds).

@95-martin-orion 95-martin-orion self-assigned this Feb 17, 2022
@daxfohl
Copy link
Collaborator

daxfohl commented Feb 18, 2022

May be worth looking at #4449 simultaneously. The qubits are there now, just need to get pushed into Result.

@MichaelBroughton MichaelBroughton added area/measurements triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on triage/discuss Needs decision / discussion, bring these up during Cirq Cynque and removed triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on labels Feb 22, 2022
@vtomole vtomole added triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on and removed triage/discuss Needs decision / discussion, bring these up during Cirq Cynque labels Feb 23, 2022
CirqBot pushed a commit that referenced this issue Feb 24, 2022
Fixes #5000.

This PR reinstates support for 2D measurement data from `_run`, but logs a warning if that path is used. External simulators will need to modify their `_run` implementation before v0.15.
95-martin-orion added a commit to 95-martin-orion/Cirq that referenced this issue Mar 2, 2022
Fixes quantumlib#5000.

This PR reinstates support for 2D measurement data from `_run`, but logs a warning if that path is used. External simulators will need to modify their `_run` implementation before v0.15.
rht pushed a commit to rht/Cirq that referenced this issue May 1, 2023
Fixes quantumlib#5000.

This PR reinstates support for 2D measurement data from `_run`, but logs a warning if that path is used. External simulators will need to modify their `_run` implementation before v0.15.
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this issue Oct 31, 2024
Fixes quantumlib#5000.

This PR reinstates support for 2D measurement data from `_run`, but logs a warning if that path is used. External simulators will need to modify their `_run` implementation before v0.15.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/measurements kind/bug-report Something doesn't seem to work. triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants