-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Allow repeated measurement keys #4899
Conversation
Update: #4555 has been merged. |
@95-martin-orion integrated. Should be gtg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is ready to go, but there's one last thing I want to be certain of.
_allow_repeated: If True, adds extra dimension to the result, | ||
corresponding to the number of times a key is repeated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, this is required for backwards-compatibility, since SimulatorBase._run
expects a 3D array to feed into the result records. Is that correct? Is there anything else motivating this addition?
Asking because it feels unwieldy, and I'd like to avoid it if it's not too complicated to do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, so this is StepResult.sample_meaurement_ops
. Since it is public, I didn't want to change the return type or behavior. It's called from SimulatorBase._run
. Options I considered were:
- Create a separate method for 3D arrays that allows duplicates
- Skipping this option in
SimulatorBase._run
and having everything go through the full-simulation path (would be much slower this way I believe) - Just inlining the logic here into
SimulatorBase._run
This seemed like the most reasonable solution, but either of the others would work too. It would be nice to eventually (or immediately) make the breaking change such that everything returns a 3D array instead of 2D. If we want to continue to offer both 2D and 3D support indefinitely then maybe they should be separate functions instead of parameter-dependent. I'm also fully open to making the parameter non-private.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM.
_allow_repeated: If True, adds extra dimension to the result, | ||
corresponding to the number of times a key is repeated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM.
This may be causing issues in external simulators: #5000 |
Allow repeated measurement keys. Implements steps 1, 2, 3, and 4 from quantumlib#4274 (comment): 1. Expand ClassicalDataStore with the extra dimension 2. Remove the exception if a measurement is repeated and just toss it into the extra dimension. 3. Ensure the generated `log_of_measurement_results` grabs the most recent index. (This step "just worked" by virtue of having a default index=-1). 4. Add an optional `index` property to `KeyCondition` to specify which index to use in classical controls (default `-1`). Step 5 will be a surprisingly straightforward follow-up to this one daxfohl/Cirq@repeated...daxfohl:repeated2 (still needs fleshed out for serializing the new field etc.). Step 6 is being done in parallel in quantumlib#4949 and subsequent PRs.
Allow repeated measurement keys. Implements steps 1, 2, 3, and 4 from quantumlib#4274 (comment): 1. Expand ClassicalDataStore with the extra dimension 2. Remove the exception if a measurement is repeated and just toss it into the extra dimension. 3. Ensure the generated `log_of_measurement_results` grabs the most recent index. (This step "just worked" by virtue of having a default index=-1). 4. Add an optional `index` property to `KeyCondition` to specify which index to use in classical controls (default `-1`). Step 5 will be a surprisingly straightforward follow-up to this one daxfohl/Cirq@repeated...daxfohl:repeated2 (still needs fleshed out for serializing the new field etc.). Step 6 is being done in parallel in quantumlib#4949 and subsequent PRs.
Allow repeated measurement keys. Implements steps 1, 2, 3, and 4 from quantumlib#4274 (comment): 1. Expand ClassicalDataStore with the extra dimension 2. Remove the exception if a measurement is repeated and just toss it into the extra dimension. 3. Ensure the generated `log_of_measurement_results` grabs the most recent index. (This step "just worked" by virtue of having a default index=-1). 4. Add an optional `index` property to `KeyCondition` to specify which index to use in classical controls (default `-1`). Step 5 will be a surprisingly straightforward follow-up to this one daxfohl/Cirq@repeated...daxfohl:repeated2 (still needs fleshed out for serializing the new field etc.). Step 6 is being done in parallel in quantumlib#4949 and subsequent PRs.
Allow repeated measurement keys.
Implements steps 1, 2, 3, and 4 from #4274 (comment):
log_of_measurement_results
grabs the most recent index. (This step "just worked" by virtue of having a default index=-1).index
property toKeyCondition
to specify which index to use in classical controls (default-1
).Step 5 will be a surprisingly straightforward follow-up to this one daxfohl/Cirq@repeated...daxfohl:repeated2 (still needs fleshed out for serializing the new field etc.).
Step 6 is being done in parallel in #4949 and subsequent PRs.