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

Add ClassicalDataStore class to keep track of qubits measured #4781

Merged
merged 84 commits into from
Feb 7, 2022

Conversation

daxfohl
Copy link
Contributor

@daxfohl daxfohl commented Dec 24, 2021

Adds a ClassicalDataStore class so we can keep track of which qubits are associated to which measurements.

Closes #3232. Initially this was created as part 14 (of 14) of https://tinyurl.com/cirq-feedforward to enable qudits in classical conditions, by storing and using dimensions of the measured qubits when calculating the integer value of each measurement when resolving sympy expressions. However it may have broader applicability.

This approach also sets us up to more easily add different types of measurements (#3233, #4274). It will also ease the path to #3002 and #4449., as we can eventually pass this into Result rather than the raw log_of_measurement_results dictionary. (The return type of _run will have to be changed to Sequence[C;assicalDataStoreReader].

Related: #887, #3231 (open question @95-martin-orion whether this closes those or not)

This PR contains a ClassicalDataStoreReader and ClassicalDataStoreBase parent "interface" for the ClassicalDataStore class as well. This will allow us to swap in different representations that may have different performance characteristics. See #3808 for an example use case. This could be done by adding an optional ClassicalDataStore factory method argument to the SimulatorBase initializer, or separately to sampler classes.

(Note this is an alternative to #4778 for supporting qudits in sympy classical control expressions, as discussed here: https://github.com/quantumlib/Cirq/pull/4778/files#r774816995. The other PR was simpler and less invasive, but a bit hacky. I felt even though bigger, this seemed like the better approach and especially fits better with our future direction, and closed the other one).

Breaking Changes:

  1. The abstract method SimulatorBase._create_partial_act_on_args argument log_of_measurement_results: Dict has been changed to classical_data: ClassicalData. Any third-party simulators that inherit SimulatorBase will need to update their implementation accordingly.
  2. The abstract base class ActOnArgs.__init__ argument log_of_measurement_results: Dict is now copied before use. For users that depend on the pass-by-reference semantics (this should be rare), they can use the new classical_data: ClassicalData argument instead, which is pass-by-reference.

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.

All files reviewed.

cirq-core/cirq/value/classical_data.py Show resolved Hide resolved
cirq-core/cirq/value/classical_data.py Outdated Show resolved Hide resolved
cirq-core/cirq/value/classical_data.py Show resolved Hide resolved
cirq-core/cirq/value/classical_data.py Show resolved Hide resolved
cirq-core/cirq/value/classical_data.py Show resolved Hide resolved
cirq-core/cirq/value/measurement_key.py Show resolved Hide resolved
cirq-core/cirq/sim/clifford/clifford_simulator_test.py Outdated Show resolved Hide resolved
cirq-core/cirq/sim/operation_target.py Show resolved Hide resolved
cirq-core/cirq/value/classical_data_test.py Show resolved Hide resolved
@daxfohl
Copy link
Contributor Author

daxfohl commented Jan 29, 2022

@95-martin-orion I think try to get #4847 merged before this one, as it'll create some conflicts for the new joiner otherwise.

@95-martin-orion
Copy link
Collaborator

@95-martin-orion I think try to get #4847 merged before this one, as it'll create some conflicts for the new joiner otherwise.

#4847 is now merged.

@daxfohl daxfohl requested a review from verult as a code owner February 3, 2022 04:12
@95-martin-orion 95-martin-orion added the BREAKING CHANGE For pull requests that are important to mention in release notes. label Feb 4, 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.

All of my comments have been addressed.

To help whoever handles the next release, could you edit the first comment in the PR to also include notes on what is broken by this change? Thanks!

@daxfohl
Copy link
Contributor Author

daxfohl commented Feb 4, 2022

@95-martin-orion added, lmk if you think it needs more details.

@95-martin-orion
Copy link
Collaborator

Looks good to me! To put my 2¢ in on why these breaking changes are OK:

  1. SimulatorBase is an aggregation of behavior shared by Cirq-internal simulators. External simulators are not expected to inherit from it, and are instead directed to inherit from the SimulatesFoo interfaces.
  2. ActOnArgs is an internal type for handling simulator state. I don't expect references to this type outside of Cirq, much less reliance on specific behavior of one of its arguments.

...so on the distant chance that you're reading this and were broken by this change, feel free to CC me on the issue.

@95-martin-orion 95-martin-orion added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Feb 7, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Feb 7, 2022
@CirqBot CirqBot merged commit 6937e41 into quantumlib:master Feb 7, 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 7, 2022
@daxfohl daxfohl deleted the qudits2 branch February 7, 2022 20:33
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
…mlib#4781)

Adds a `ClassicalDataStore` class so we can keep track of which qubits are associated to which measurements.

Closes quantumlib#3232. Initially this was created as part 14 (of 14) of https://tinyurl.com/cirq-feedforward to enable qudits in classical conditions, by storing and using dimensions of the measured qubits when calculating the integer value of each measurement when resolving sympy expressions. However it may have broader applicability.

This approach also sets us up to more easily add different types of measurements (quantumlib#3233, quantumlib#4274). It will also ease the path to quantumlib#3002 and quantumlib#4449., as we can eventually pass this into `Result` rather than the raw `log_of_measurement_results` dictionary. (The return type of `_run` will have to be changed to `Sequence[C;assicalDataStoreReader]`.

Related: quantumlib#887, quantumlib#3231 (open question @95-martin-orion whether this closes those or not)

This PR contains a `ClassicalDataStoreReader` and `ClassicalDataStoreBase` parent "interface" for the `ClassicalDataStore` class as well. This will allow us to swap in different representations that may have different performance characteristics. See quantumlib#3808 for an example use case. This could be done by adding an optional `ClassicalDataStore` factory method argument to the `SimulatorBase` initializer, or separately to sampler classes.

(Note this is an alternative to quantumlib#4778 for supporting qudits in sympy classical control expressions, as discussed here: https://github.com/quantumlib/Cirq/pull/4778/files#r774816995. The other PR was simpler and less invasive, but a bit hacky. I felt even though bigger, this seemed like the better approach and especially fits better with our future direction, and closed the other one).

**Breaking Changes**:
1. The abstract method `SimulatorBase._create_partial_act_on_args` argument `log_of_measurement_results: Dict` has been changed to `classical_data: ClassicalData`. Any third-party simulators that inherit `SimulatorBase` will need to update their implementation accordingly.
2. The abstract base class `ActOnArgs.__init__` argument `log_of_measurement_results: Dict` is now copied before use. For users that depend on the pass-by-reference semantics (this should be rare), they can use the new `classical_data: ClassicalData` argument instead, which is pass-by-reference.
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this pull request Oct 31, 2024
…mlib#4781)

Adds a `ClassicalDataStore` class so we can keep track of which qubits are associated to which measurements.

Closes quantumlib#3232. Initially this was created as part 14 (of 14) of https://tinyurl.com/cirq-feedforward to enable qudits in classical conditions, by storing and using dimensions of the measured qubits when calculating the integer value of each measurement when resolving sympy expressions. However it may have broader applicability.

This approach also sets us up to more easily add different types of measurements (quantumlib#3233, quantumlib#4274). It will also ease the path to quantumlib#3002 and quantumlib#4449., as we can eventually pass this into `Result` rather than the raw `log_of_measurement_results` dictionary. (The return type of `_run` will have to be changed to `Sequence[C;assicalDataStoreReader]`.

Related: quantumlib#887, quantumlib#3231 (open question @95-martin-orion whether this closes those or not)

This PR contains a `ClassicalDataStoreReader` and `ClassicalDataStoreBase` parent "interface" for the `ClassicalDataStore` class as well. This will allow us to swap in different representations that may have different performance characteristics. See quantumlib#3808 for an example use case. This could be done by adding an optional `ClassicalDataStore` factory method argument to the `SimulatorBase` initializer, or separately to sampler classes.

(Note this is an alternative to quantumlib#4778 for supporting qudits in sympy classical control expressions, as discussed here: https://github.com/quantumlib/Cirq/pull/4778/files#r774816995. The other PR was simpler and less invasive, but a bit hacky. I felt even though bigger, this seemed like the better approach and especially fits better with our future direction, and closed the other one).

**Breaking Changes**:
1. The abstract method `SimulatorBase._create_partial_act_on_args` argument `log_of_measurement_results: Dict` has been changed to `classical_data: ClassicalData`. Any third-party simulators that inherit `SimulatorBase` will need to update their implementation accordingly.
2. The abstract base class `ActOnArgs.__init__` argument `log_of_measurement_results: Dict` is now copied before use. For users that depend on the pass-by-reference semantics (this should be rare), they can use the new `classical_data: ClassicalData` argument instead, which is pass-by-reference.
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.

Classical Control: Feedforward
4 participants