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

Extract SV/DM/CliffordSimState from simulators #5260

Merged
merged 17 commits into from
Apr 21, 2022

Conversation

daxfohl
Copy link
Contributor

@daxfohl daxfohl commented Apr 14, 2022

First part of #5244, so that we don't have two variants of "XYZSimulatorState".

@95-martin-orion

As mentioned in a comment to that issue, these sim states aren't exposed to the end user (except CliffordState), and end up being an unnecessary layer that wraps the ActOnArgs used in the simulation. This PR removes them from the simulation, and instead uses the ActOnArgs (specifically the OperationTarget[ActOnArgs]) directly. This allows us simply to deprecate and remove those classes in the future. (For Clifford, we also remove it from the simulation logic, and only create the CliffordState object in the public functions that expose it).

There are two distinct parts to this PR. The first was to move some of the shared logic between ActOnArgs and ActOnArgsContainer down a level in the hierarchy to OperationTarget. (Primarily this was to give a common qubit_map property rather than having to create one from the qubits each time it was required subsequently.

The second part is the migration from using the existing XYZSimState classes to using the ActOnArgs directly. The root of this is in StepResultBase._simulator_state. Previously each XYZStepResult defined its own _simulator_state property, that created the XYZSimState we're trying to extract. This PR deletes those and replaces them with the new StepResultBase._simulator_state, which returns the OperationTarget[TActOnArgs] instead. That bubbles up into _final_simulator_state, and the various functions in the XYZStepResults and XYZTrialResults have been modified to account for that change.

Finally, since the simulators are no longer bound to these XYZSimulatorStates anymore, TSimulatorState is removed from the type parameters everywhere possible.

@CirqBot CirqBot added the size: L 250< lines changed <1000 label Apr 14, 2022
@daxfohl daxfohl marked this pull request as ready for review April 14, 2022 18:59
@daxfohl daxfohl requested review from a team, vtomole and cduck as code owners April 14, 2022 18:59
@daxfohl daxfohl requested a review from viathor April 14, 2022 18:59
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.

Couple of concerns with nontrivial method changes, but on the whole this is definitely looking nicer.

cirq-core/cirq/sim/simulator.py Outdated Show resolved Hide resolved
cirq-core/cirq/sim/simulator.py Show resolved Hide resolved
cirq-core/cirq/sim/state_vector_simulator.py Show resolved Hide resolved
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.

Looks solid to me.

@95-martin-orion 95-martin-orion added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Apr 21, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Apr 21, 2022
@CirqBot CirqBot merged commit 32eba73 into quantumlib:master Apr 21, 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 Apr 21, 2022
@daxfohl daxfohl deleted the no-sim-state branch April 21, 2022 16:23
tonybruguier pushed a commit to tonybruguier/Cirq that referenced this pull request Apr 22, 2022
First part of quantumlib#5244, so that we don't have two variants of "XYZSimulatorState".

@95-martin-orion 

As mentioned in a comment to that issue, these sim states aren't exposed to the end user (except CliffordState), and end up being an unnecessary layer that wraps the ActOnArgs used in the simulation. This PR removes them from the simulation, and instead uses the ActOnArgs (specifically the OperationTarget[ActOnArgs]) directly. This allows us simply to deprecate and remove those classes in the future. (For Clifford, we also remove it from the simulation logic, and only create the CliffordState object in the public functions that expose it).

There are two distinct parts to this PR. The first was to move some of the shared logic between ActOnArgs and ActOnArgsContainer down a level in the hierarchy to OperationTarget. (*Primarily* this was to give a common `qubit_map` property rather than having to create one from the `qubits` each time it was required subsequently.

The second part is the migration from using the existing XYZSimState classes to using the ActOnArgs directly. The root of this is in `StepResultBase._simulator_state`. Previously each XYZStepResult defined its own `_simulator_state` property, that created the XYZSimState we're trying to extract. This PR deletes those and replaces them with the new `StepResultBase._simulator_state`, which returns the `OperationTarget[TActOnArgs]` instead. That bubbles up into `_final_simulator_state`, and the various functions in the XYZStepResults and XYZTrialResults have been modified to account for that change.

Finally, since the simulators are no longer bound to these `XYZSimulatorStates` anymore, `TSimulatorState` is removed from the type parameters everywhere possible.
CirqBot pushed a commit that referenced this pull request Apr 22, 2022
Follow-up to #5260, as that PR makes final_step_result unnecessary.

Also deprecate DensityMatrixSimulatorState and StateVectorSimulatorState

Closes #5271
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
First part of quantumlib#5244, so that we don't have two variants of "XYZSimulatorState".

@95-martin-orion 

As mentioned in a comment to that issue, these sim states aren't exposed to the end user (except CliffordState), and end up being an unnecessary layer that wraps the ActOnArgs used in the simulation. This PR removes them from the simulation, and instead uses the ActOnArgs (specifically the OperationTarget[ActOnArgs]) directly. This allows us simply to deprecate and remove those classes in the future. (For Clifford, we also remove it from the simulation logic, and only create the CliffordState object in the public functions that expose it).

There are two distinct parts to this PR. The first was to move some of the shared logic between ActOnArgs and ActOnArgsContainer down a level in the hierarchy to OperationTarget. (*Primarily* this was to give a common `qubit_map` property rather than having to create one from the `qubits` each time it was required subsequently.

The second part is the migration from using the existing XYZSimState classes to using the ActOnArgs directly. The root of this is in `StepResultBase._simulator_state`. Previously each XYZStepResult defined its own `_simulator_state` property, that created the XYZSimState we're trying to extract. This PR deletes those and replaces them with the new `StepResultBase._simulator_state`, which returns the `OperationTarget[TActOnArgs]` instead. That bubbles up into `_final_simulator_state`, and the various functions in the XYZStepResults and XYZTrialResults have been modified to account for that change.

Finally, since the simulators are no longer bound to these `XYZSimulatorStates` anymore, `TSimulatorState` is removed from the type parameters everywhere possible.
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
…5281)

Follow-up to quantumlib#5260, as that PR makes final_step_result unnecessary.

Also deprecate DensityMatrixSimulatorState and StateVectorSimulatorState

Closes quantumlib#5271
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: L 250< lines changed <1000
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants