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

Support repeated measurement keys in cirq.Result #4555

Merged
merged 3 commits into from
Feb 14, 2022

Conversation

maffoo
Copy link
Contributor

@maffoo maffoo commented Oct 5, 2021

Part of #4274

@google-cla google-cla bot added the cla: yes Makes googlebot stop complaining. label Oct 5, 2021
@CirqBot CirqBot added the size: M 50< lines changed <250 label Oct 5, 2021
@maffoo maffoo force-pushed the u/maffoo/repeated-keys branch from d06caa7 to 669bf13 Compare October 14, 2021 19:58
@maffoo maffoo force-pushed the u/maffoo/repeated-keys branch from 669bf13 to e080647 Compare November 20, 2021 00:24
@maffoo maffoo force-pushed the u/maffoo/repeated-keys branch 4 times, most recently from 2614a78 to fb17c43 Compare January 14, 2022 20:26
@maffoo maffoo force-pushed the u/maffoo/repeated-keys branch from fb17c43 to f71da9d Compare January 18, 2022 19:59
@maffoo maffoo force-pushed the u/maffoo/repeated-keys branch from f71da9d to 234807e Compare January 26, 2022 20:27
@maffoo
Copy link
Contributor Author

maffoo commented Jan 26, 2022

I've rebased this on master including the separation of cirq.Result into an ABC. One design question here is what the new interface for accessing 3D arrays of repeated measurements should look like. I've called in measurements2 as a placeholder. Any thoughts @95-martin-orion?

@95-martin-orion
Copy link
Collaborator

One design question here is what the new interface for accessing 3D arrays of repeated measurements should look like.

Let's cover this at our meeting Thursday - there are a bunch of open threads here and I'd like to get them woven together before rushing ahead on this.

@maffoo maffoo force-pushed the u/maffoo/repeated-keys branch 3 times, most recently from 064b8ca to faa7446 Compare January 31, 2022 20:42
@maffoo maffoo force-pushed the u/maffoo/repeated-keys branch from faa7446 to ccab511 Compare January 31, 2022 22:03
@maffoo maffoo marked this pull request as ready for review January 31, 2022 22:04
@maffoo maffoo requested review from cduck, vtomole and a team as code owners January 31, 2022 22:04
@95-martin-orion 95-martin-orion self-assigned this Feb 2, 2022
Copy link
Collaborator

@95-martin-orion 95-martin-orion left a comment

Choose a reason for hiding this comment

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

A pretty self-explanatory implementation of Result as described in the RFC, though there's a comment from @Strilanc there you should take a look at before we commit to this.

prev_n = num_qubits.setdefault(key, n)
if n != prev_n:
raise ValueError(
"Different num qubits for repeated measurement: "
Copy link
Collaborator

Choose a reason for hiding this comment

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

Two things:

  • This also needs to check that the dimensions for each qudit are the same - e.g. (3, 2) != (2, 3).
  • This feels like the wrong place for this check to live, since presumably every sampler/simulator is going to require it. Could we move it up to the Sampler superclass?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this into a helper function Sampler._get_measurement_shapes in the base class. Note that this is basically a preliminary version of the "measurement_key_shape" protocol proposed in #4888.

@95-martin-orion
Copy link
Collaborator

@maffoo Heads up: due #4899 requiring access to Result.records and needing to be submitted before the v0.14 in a couple weeks, we need to merge this PR as soon as possible.

@95-martin-orion
Copy link
Collaborator

Approved, but requires conflict resolution before merging. The change looks fairly minor.

@maffoo maffoo added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Feb 14, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Feb 14, 2022
@CirqBot CirqBot merged commit dd55a86 into master Feb 14, 2022
@CirqBot CirqBot removed the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Feb 14, 2022
@CirqBot CirqBot deleted the u/maffoo/repeated-keys branch February 14, 2022 23:45
@CirqBot CirqBot removed the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Feb 14, 2022
@daxfohl
Copy link
Contributor

daxfohl commented Feb 15, 2022

I noticed one surprising (to me anyway) behavior of ResultDict is that, when passing in records but not measurements, and any measurement occurs more than once, then ResultDict.measurements['key'] fails even if key was only measured once.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Makes googlebot stop complaining. size: M 50< lines changed <250
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants