-
Notifications
You must be signed in to change notification settings - Fork 1.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
Implement _act_on_ for SingleQubitCliffordGate #7069
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7069 +/- ##
==========================================
- Coverage 98.18% 98.15% -0.03%
==========================================
Files 1089 1089
Lines 95208 95319 +111
==========================================
+ Hits 93478 93563 +85
- Misses 1730 1756 +26 ☔ View full report in Codecov by Sentry. |
Actually, what probably makes the most sense, is to define new protocols This would also be more aligned with how the rest of the simulators work: other All that said, that's a project in its own right, going beyond the scope of the issue #5256, and would be its own issue, likely a good part-time project for somebody. The current PR is more a direct fix for #5256, and will help speed up both of the Clifford simulators for these gates in the meantime. (Note: probably worth a perf test to confirm). |
Oof, yeah, so in this PR, if we remove the implementation of For CH form, there's no difference between the slow cases, as |
Oh, wait. I see. The perf of I'll switch it back to returning |
@@ -143,10 +143,6 @@ def _gate_tableau(num_qubits: int, gate: raw_types.Gate) -> 'cirq.CliffordTablea | |||
class CommonCliffordGateMetaClass(value.ABCMetaImplementAnyOneOf): | |||
"""A metaclass used to lazy initialize several common Clifford Gate as class attributes.""" | |||
|
|||
# These are class properties so we define them as properties on a metaclass. | |||
# Note that in python 3.9+ @classmethod can be used with @property, so these | |||
# can be moved to CommonCliffordGates. |
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.
This ability was removed again in 3.11, so deleting the comment. https://stackoverflow.com/questions/128573/using-property-on-classmethods/64738850#64738850
Maybe fixes #5256
@BichengYing is this something like what you were looking for?
As background on the sim architecture, there's two levels:
SimulationState
, andQuantumStateRepresentation
.QuantumStateRepresentation
is a container for the quantum state piece only, so there's implementations forBufferedStateVector
,BufferedDensityMatrix
,CliffordTableau
,ChForm
, and others. The latter two inherit fromStabilizerState
, which provides the interfaceapply_x(axis, exponent, phase)
,apply_y
, etc that those two subclasses implement. This layer is also meant to be low-level and only works on axes/indexes and unitaries/channels/tableaux, notQid
s orGate
s.SimulationState
is a layer above that, and it contains aQuantumStateRepresentation
, as well as the RNG, a place to store measurements that have been taken, and the qubit->index mapping. This layer has a parallel type hierarchy,StateVectorSimulationState
,StabilizerSimulationState
,CliffordTableauSimulationState
, and so on, which I don't love, but was hard to avoid. This is the layer that_act_on_
/_act_on_fallback_
works with. So that's why_act_on_
has the annoyingsim_state._state
.There's also a
Simulator
layer on top of that, but that's out of scope for this topic._act_on_
then, is mostly used for delegation. If you search fordef _act_on_(
, you can see most of the implementations are for "composite" ops that delegate to sub-ops.CircuitOperation
,ClassicallyControlledOperation
,TaggedOperation
, and so on. But in most cases,_act_on_fallback_
defined in theSimulationState
itself knows how to handle the op efficiently. So for instanceStateVectorSimulationState
tries theapply_unitary
protocol first, whichXPowGate
has an efficient implementation for, that's faster than generating the unitary matrix and applying it generically. SoXPowGate
does not need an explicit_act_on_
implementation to achieve this efficiency increase. So in general, we've tried to avoid havingGate
classes need to be aware of any specificSimulationState
classes, or do anyif isinstance(sim_state, XYZSimState)
in_act_on_
implementations.I think
SingleQubitCliffordGate
is different in this regard: it's specifically designed to be used only in Clifford simulators, and so perhaps having an explicitisinstance(sim_state, ...)
in an_act_on_
makes sense. That's what this PR does. Another approach could be to make theStabilizerSimulationState
aware of those gates in_strat_apply_gate
. That approach flips the dependency upside-down, so that the simulator is aware of the gate, rather than the gate being aware of the simulator. A third approach could be that we makeCliffordGates
the only thing thatStabilizerSimulationState
is aware of, remove the special-case handling of e.g.XPowGate
, and change the_strat_decompose
into_strat_decompose_into_cliffords
, such thatXPowGate
gets decomposed intoCliffordGate.X
before it can be applied....Now that I think about it, the third strategy may make the most sense: have Clifford simulators only work with Clifford gates and things that decompose into Cliffords. That way we could maybe get rid of the
apply_x
,apply_y
etc. of theStabilizerState
interface and replace them all with something more generic. (I'm not all that familiar with Cliffords, so you'd probably know more about that than me).Anyway, the current PR is the lowest-hanging quick fix for #5256. LMK if this is what you were looking for.