Rename ActOnArgs to SimulationState #5293

daxfohl · 2022-04-26T14:13:44Z

Most of #5244. Will backfill the existing ActOnArgs classes in subsequent PR, as it will merge cleaner (here git detects the renamed file; if we do backfill here then git will see all new files). Will also need to rename a few methods like _create_partial_act_on_args in a future PR.

daxfohl · 2022-04-26T17:40:38Z

@95-martin-orion

95-martin-orion

This will be a big help in parsing these types! How are we doing in terms of remaining "act_on_arg" / "actonarg" appearances in Cirq after this PR?

Thanks also for keeping this nice for review. For the follow-up PRs, I think the appropriate way to handle them is to point them at this branch - that way I can approve those and get the whole sequence merged into master without a big breaking change.

cirq-core/cirq/ops/clifford_gate.py

95-martin-orion · 2022-04-26T20:34:59Z

cirq-core/cirq/circuits/circuit_operation.py

        return self.mapped_circuit(deep=False).all_operations()

-    def _act_on_(self, args: 'cirq.OperationTarget') -> bool:
+    def _act_on_(self, args: 'cirq.SimulationStateBase') -> bool:


A minor nit: renaming args to state (or similar) throughout this PR would be nice.

I did for local variables. For function arguments I expect there'll need to be a deprecation cycle. Though I guess for private / protocol methods that doesn't apply.

I went ahead and updated it for the protocol handlers. I think I'm going to have to take a rain check on the rest as there's just too much to fit in one PR, and args is way too common to find and replace. The main one is _create_partial_act_on_args, which is a bit complicated and will require a deprecation cycle and probably some dynamic hasattr or inspect stuff to be backwards compatible. Beyond that, misc stuff here and there, mostly in sim.. Was there anything else in particular you wanted me to deal with in this PR?

Good call on the deprecation cycle. We should keep an eye on it even for the protocol methods, since they have a bad habit of being called from outside of Cirq.

I don't see anything else urgent to deal with in this PR - moving to #5296 for further review.

cirq-core/cirq/sim/clifford/stabilizer_simulation_state.py

cirq-core/cirq/sim/density_matrix_simulation_state.py

cirq-core/cirq/sim/simulation_state.py

cirq-core/cirq/sim/simulator_base_test.py

95-martin-orion · 2022-04-27T14:40:30Z

Ready to go once #5296 is merged into this.

Add old ActOnArgs, deprecated

daxfohl · 2022-04-27T14:50:31Z

@95-martin-orion looks like that approach still caused git to change from recognizing renamed files to big unrelated deletes and adds. But I guess that's fine since we're done reviewing.

95-martin-orion · 2022-04-27T14:55:50Z

@95-martin-orion looks like that approach still caused git to change from recognizing renamed files to big unrelated deletes and adds. But I guess that's fine since we're done reviewing.

For posterity, the renaming view of this PR can be seen by only looking at commits up to 7486e3b (link).

CirqBot · 2022-04-27T14:56:55Z

Automerge cancelled: A required status check is not present.

Missing statuses: ['Build docs', 'Build protos', 'Changed files test', 'Coverage check', 'Doc test', 'Format check', 'Lint check', 'Misc check', 'Notebook formatting', 'Pytest MacOS (3.7)', 'Pytest MacOS (3.8)', 'Pytest MacOS (3.9)', 'Pytest Ubuntu (3.7)', 'Pytest Ubuntu (3.8)', 'Pytest Ubuntu (3.9)', 'Pytest Windows (3.7)', 'Pytest Windows (3.8)', 'Pytest Windows (3.9)', 'Type check', 'Typescript lint check', 'Typescript tests', 'Typescript tests coverage']

daxfohl · 2022-04-27T15:31:03Z

@95-martin-orion kick me

tanujkhattar · 2022-09-08T22:45:33Z

cirq-core/cirq/sim/simulation_state_base.py

+TSimulationState = TypeVar('TSimulationState', bound='cirq.SimulationState')
+
+
+class SimulationStateBase(Generic[TSimulationState], metaclass=abc.ABCMeta):


@daxfohl Can you please explain what's going on with the generics here? For example:

SimulationStateBase(Generic[TSimulationState], metaclass=abc.ABCMeta) is a generic of type TSimulationState = TypeVar('TSimulationState', bound='cirq.SimulationState')` and

SimulationState(SimulationStateBase, Generic[TState], metaclass=abc.ABCMeta) derives from SimulationStateBase and is also a generic of type TypeVar('TState', bound='cirq.QuantumStateRepresentation')

Why is TSimulationState used in SimulatorStateBase bound to SimulatorState and not SimulatorStateBase ? Why can we not use TSelfTarget in place of TSimulationState in the SimulationStateBase class? Is it because we want to allow __getitem__ and create_merged_state to potentially return a different type compared to TSelfTarget ?

Thanks for asking. It's pretty challenging and I remember some idiosyncrasies of mypy getting in the way for a while.

So hierarchy: SimStateBase has two subclasses {SimState, SimProductState}.

SimState is abstract and has multiple subclasses: StateVectorSimState, DensityMatrixSimState, etc. Those are "dense" as in they have a single big state vector, density matrix, tableau, or whatever for all qubits in the simulation. TSimState is the generic constrained to a single SimState.

SimProductState is concrete and generic: SimProductState[TSimState]. It contains a qubit->TSimState mapping. It's a product state representation for whichever TSimState it contains, so it's smaller and faster when there's not full entanglement. I consider this a "final" class. (You can control whether a simulator uses a SimProductState or a single SimState, by the split_untangled_states flag in simulator constructors that support it).

SimStateBase is the superclass of them all. It is generic on TSimState, because there's an abstract function create_merged_state() -> TSimState that's there to give you the "dense" version of whatever your TSimState is. So in SimState, the implementation of that just returns self; in SimProductState it krons up all the values in its mapping and returns that. This is why SimStateBase is generic on TSimState, and why TSimState is bound to SimState: SimState is the "dense" representation. __getitem__ works similarly; the docstring should probably read "Gets the dense substate associated with the qubit" or something like that.

Then TSelfTarget is different because the copy of a SimProductState should return a SimProductState.

And TState is separate. It is purely the quantum component. SimState contains not only the quantum component (density matrix, tableau, etc), but also the qubit->axis mapping, a prng, measurement records, etc. So that's why you see DensityMatrixSimState inherits SimState[DensityMatrixQuantumRepresentation].

In an ideal world we shouldn't even need DensityMatrixSimState etc as their own classes, and instead just have SimState be a generic final class that we use directly. However there are a lot of backwards compatibility issues and useful convenience methods preventing that. (Otherwise you'd end up needing to write code like result.get_merged_state().state.state.state_vector or something).

Thanks, this is extremely helpful!

I think the fact that we are supporting SimulationProductState for any arbitrary TSimulationState is adding a lot of complexity to the type hierarchy. An alternative could have been to have

A type Hierarchy for specifying simulation states as Generic[TState]; i.e. what the SimulationState class is doing right now. SimulationStateBase wouldn't exist and the abstract base class for simulation states would be SimulationState.

Have a separate container type for dense vs sparse representation of states -- This container could maintain the Dict[cirq.Qid, TSimulationState]. Based on the configurable parameter, a simulator would construct either a dense container or a sparse container for the specified simulation state type.

The fact that a container of simulation states (SimulationProductState) is also a simulation state is I think making the type hierarchy super complex. But given the Cirq 1.0 backwards compatibility constraints, I'm not sure if we can do anything about the situation at this point except improving documentation.

Yes, that was our initial approach but that caused some backwards incompatibility at the time IIRC, so we went with subclassing. Probably could have revisited this before 1.0 but didn't think about it.

daxfohl · 2022-09-13T02:49:54Z

Note the other option is to push the "product state" functionality down to the quantum-state level rather than the simulation-state level. I actually think that would be the best approach (it maps better to what's physically happening). I investigated this approach before 1.0 in #5389, but my conclusion was that it would be a major project.

* Rename ActOnArgs * Rename act_on_args locals and tests * Rename modules * coverage * coverage * TActOnArgs * change args to sim_state in _act_on_ impls * Add old ActOnArgs, deprecated Co-authored-by: Cirq Bot <craiggidney+github+cirqbot@google.com>

daxfohl added 4 commits April 26, 2022 06:50

Rename ActOnArgs

181aab5

Rename act_on_args locals and tests

77612c5

Rename modules

a640c6c

coverage

ddf099f

CirqBot added the size: L 250< lines changed <1000 label Apr 26, 2022

coverage

52e6bc7

daxfohl marked this pull request as ready for review April 26, 2022 17:40

daxfohl requested review from a team, cduck, verult, vtomole and wcourtney as code owners April 26, 2022 17:40

daxfohl requested a review from dstrain115 April 26, 2022 17:40

95-martin-orion self-assigned this Apr 26, 2022

95-martin-orion requested changes Apr 26, 2022

View reviewed changes

daxfohl added 3 commits April 26, 2022 19:10

TActOnArgs

fc676df

change args to sim_state in _act_on_ impls

7486e3b

Add old ActOnArgs, deprecated

fc5ddeb

daxfohl mentioned this pull request Apr 27, 2022

Readd old ActOnArgs, but deprecated #5296

Closed

Merge pull request #4 from daxfohl/actonargs-2

de10ee5

Add old ActOnArgs, deprecated

95-martin-orion approved these changes Apr 27, 2022

View reviewed changes

95-martin-orion added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Apr 27, 2022

CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Apr 27, 2022

Merge branch 'master' into actonargs-1

fc649cc

daxfohl mentioned this pull request Apr 27, 2022

Rename ActOnXYZArgs to (say) XYZSimulationState #5244

Closed

3 tasks

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 27, 2022

95-martin-orion merged commit d0422d1 into quantumlib:master Apr 27, 2022

daxfohl deleted the actonargs-1 branch April 27, 2022 16:22

tanujkhattar reviewed Sep 8, 2022

View reviewed changes

		TSimulationState = TypeVar('TSimulationState', bound='cirq.SimulationState')


		class SimulationStateBase(Generic[TSimulationState], metaclass=abc.ABCMeta):

Rename ActOnArgs to SimulationState #5293

Rename ActOnArgs to SimulationState #5293

Uh oh!

Conversation

daxfohl commented Apr 26, 2022

Uh oh!

daxfohl commented Apr 26, 2022

Uh oh!

95-martin-orion left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

95-martin-orion commented Apr 27, 2022

Uh oh!

daxfohl commented Apr 27, 2022

Uh oh!

95-martin-orion commented Apr 27, 2022

Uh oh!

CirqBot commented Apr 27, 2022

Uh oh!

daxfohl commented Apr 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxfohl commented Sep 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants