Skip to content

Permit 2D _run output for backwards compatibility. #5014

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

Merged
merged 4 commits into from
Feb 24, 2022

Conversation

95-martin-orion
Copy link
Collaborator

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 95-martin-orion requested review from a team, vtomole and cduck as code owners February 23, 2022 17:45
@CirqBot CirqBot added the size: S 10< lines changed <50 label Feb 23, 2022
@95-martin-orion
Copy link
Collaborator Author

CC @daxfohl

@daxfohl
Copy link
Collaborator

daxfohl commented Feb 23, 2022

lgtm, thanks for cleaning up my mess!

@daxfohl
Copy link
Collaborator

daxfohl commented Feb 23, 2022

Is it possible to remove the measurements argument from the ResultDict constructor now?

@95-martin-orion
Copy link
Collaborator Author

Is it possible to remove the measurements argument from the ResultDict constructor now?

There's still a bunch of other places that still use it (plus #5015, which is a similar issue), but maybe? v0.14 is coming up very soon - if we can't make the change before then, we could mark it as deprecated and remove it afterwards.

Copy link
Contributor

@maffoo maffoo left a comment

Choose a reason for hiding this comment

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

Some questions about mocking in tests, otherwise LGTM

@@ -101,6 +101,32 @@ def test_run_simulator_sweeps():
assert simulator._run.call_count == 2


@mock.patch.multiple(cirq.SimulatesSamples, __abstractmethods__=set(), _run=mock.Mock())
def test_run_simulator_sweeps_with_deprecated_run():
simulator = cirq.SimulatesSamples()
Copy link
Contributor

Choose a reason for hiding this comment

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

How hard would it be to create an actual simulator subclass for this test that has the deprecated _run behavior? Mocking always feel rather brittle to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This test is just a copy of test_run_simulator_sweeps with _run modified to return a 2D array, but it's not particularly expensive to remove mocking throughout this file - especially since Cirq on the whole seems to have moved away from mocking since this was written. I'll make the change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed some of the mocks. The intermediate state mocks are a fair bit more convoluted, so I've left them for now.

@95-martin-orion 95-martin-orion added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Feb 24, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Feb 24, 2022
@CirqBot CirqBot merged commit 43527c8 into quantumlib:master Feb 24, 2022
@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Feb 24, 2022
95-martin-orion added a commit to 95-martin-orion/Cirq that referenced this pull request 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 pull request 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 pull request 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
size: S 10< lines changed <50
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent shape of Result.records
4 participants