Fix act-on specialization for all the Clifford simulators #4748
Conversation
|
@95-martin-orion I think you can have a quick look at this too since you have the most understanding of the simulation infrastructure. The code change itself is pretty straightforward, just moving things around. The motivation being, we can remove all the specialization logic and instance checks from the gates and make some shared and reusable structure. |
|
@Strilanc do you have an opinion here? This may be somewhat related to #2423. I think the Clifford logic might have been all in the simulators prior to that issue, and then it got moved to the gates as part of the act_on protocol implementation? This PR would move the logic into the corresponding ActOnArgs themselves. I think it's a little more extensible this way, closer to other simulator designs, and we get to share some common logic as well. As implemented, it just sits on top of the standard decompose protocol (and SWAP uses that). It seems good enough for now. We could add an independent decompose_to_clifford protocol later, as mentioned in the issue, if the regular decompose is insufficient. Though I wouldn't propose doing that just yet because we may want that to sit on top of the gate pattern matching that @tanujkhattar is working on. |
| exponent = g.exponent | ||
| if exponent % 2 == 0: | ||
| return | ||
| assert exponent % 0.5 == 0.0 |
There was a problem hiding this comment.
Avoid assertions outside of test code - prefer raising an error with a useful debugging message.
There was a problem hiding this comment.
This should never trigger though. It's a private method only called by _strat_apply_gate, which tests has_stabilizer_effect first. Are we allowed to keep assertions in those cases, or do those need to change to ValueError too (or remove it entirely)?
There was a problem hiding this comment.
Our style guide places no restriction on assert vs raise-error, so this is optional. That said...
My take is that a well-written error message is much easier to debug than an assert, and on a long enough time scale:
- even "never touched" code will trigger, and
- the original authors won't be available to help debug
|
The reason it was designed to be in the individual gates was to make it possible to introduce new gates without editing cirq itself. As long as that's not broken it doesn't particularly matter to me whether the method lives in the gate or in the big if-else block. The big if-else block might be slower, but the clifford sim is already so unusably slow it's hard to worry too much about it... |
|
Yes, I was concerned whether it might be detrimental to performance as well, as this change does add some logic that happens before getting to the actual numpy code. But I tested it out on various circuit shapes and sizes and there's no discernable difference. Any new or third-party gates can still use the act_on approach for Clifford (or any other) simulators; this change doesn't break that. |
|
@95-martin-orion I updated the assertions, and changed the names of axis1, axis2 to control_axis, target_axis. This can merge if tests pass. |
95-martin-orion
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
CirqBot
removed
automerge
…#4748) Fixes quantumlib#4639, ref [tinyurl.com/clifford-simulators-refactor](https://tinyurl.com/clifford-simulators-refactor) This change makes the Clifford simulators more consistent with other simulators. Rather than the gate having all the logic of how to update the simulator state, which seemed out-of-place and limited the ability to reuse in any new kinds of Clifford simulators, this PR moves all the logic to the simulators themselves. It also creates an ABC for Clifford simulator states, allowing reuse of the evolution logic in the base class, such that the subclasses just have to implement the specific gate applicators. Note I *tried* doing this via a new Clifford protocol as well ([link](https://github.com/quantumlib/Cirq/compare/master...daxfohl:clifford3?expand=1)) as we originally discussed, and it works but I didn't like the result. The protocol ended up just duplicating all the information of the gate, in a weird structure, and felt like an extra, unnecessary, hacky wrapper around just using the gate itself. So I prefer the approach in this PR that just uses the gate instances directly, though I'm open to suggestion. @tanujkhattar @viathor (cc @ybc1991)
Add initial Clifford Gate with multiple qubits. Compared with SingleQubitCliffordGate, it has fewer functionalities since we cannot enumerate all of them with PauliGates and several special single qubit properties like Bloch rotation no longer exist. Anyway, it provides several basic interactions:
1. It uses Clifford tableau as underlying data representation (different from the state representation).
2. It can be constructed from a tableau or list of operations (`_has_stabilizer_effect_` only). All Clifford gates can be built through \{S, H, CNOT\}, so we can construct any Clifford Gate from the list of operations. We just cannot pre-define it.
3. Decomposing into several basic operations.
4. Get unitary matrix through decomposing (we cannot do this in a reverse way from unitary to Clifford gate :( ).
5. Know how to interact with ActOnCliffordTableauArgs, i.e. it should be able to use with CliffordTableau simulator (Looks like we don't have that in cirq yet? @daxfohl will add that? see #4639 and #4748.).
This PR is part of efforts for #3639. Context: this PR doesn't introduce any new algorithms but the key methods are already implemented in #4183 and #4096.
Add initial Clifford Gate with multiple qubits. Compared with SingleQubitCliffordGate, it has fewer functionalities since we cannot enumerate all of them with PauliGates and several special single qubit properties like Bloch rotation no longer exist. Anyway, it provides several basic interactions:
1. It uses Clifford tableau as underlying data representation (different from the state representation).
2. It can be constructed from a tableau or list of operations (`_has_stabilizer_effect_` only). All Clifford gates can be built through \{S, H, CNOT\}, so we can construct any Clifford Gate from the list of operations. We just cannot pre-define it.
3. Decomposing into several basic operations.
4. Get unitary matrix through decomposing (we cannot do this in a reverse way from unitary to Clifford gate :( ).
5. Know how to interact with ActOnCliffordTableauArgs, i.e. it should be able to use with CliffordTableau simulator (Looks like we don't have that in cirq yet? @daxfohl will add that? see quantumlib#4639 and quantumlib#4748.).
This PR is part of efforts for quantumlib#3639. Context: this PR doesn't introduce any new algorithms but the key methods are already implemented in quantumlib#4183 and quantumlib#4096.
…#4748) Fixes quantumlib#4639, ref [tinyurl.com/clifford-simulators-refactor](https://tinyurl.com/clifford-simulators-refactor) This change makes the Clifford simulators more consistent with other simulators. Rather than the gate having all the logic of how to update the simulator state, which seemed out-of-place and limited the ability to reuse in any new kinds of Clifford simulators, this PR moves all the logic to the simulators themselves. It also creates an ABC for Clifford simulator states, allowing reuse of the evolution logic in the base class, such that the subclasses just have to implement the specific gate applicators. Note I *tried* doing this via a new Clifford protocol as well ([link](https://github.com/quantumlib/Cirq/compare/master...daxfohl:clifford3?expand=1)) as we originally discussed, and it works but I didn't like the result. The protocol ended up just duplicating all the information of the gate, in a weird structure, and felt like an extra, unnecessary, hacky wrapper around just using the gate itself. So I prefer the approach in this PR that just uses the gate instances directly, though I'm open to suggestion. @tanujkhattar @viathor (cc @ybc1991)
Add initial Clifford Gate with multiple qubits. Compared with SingleQubitCliffordGate, it has fewer functionalities since we cannot enumerate all of them with PauliGates and several special single qubit properties like Bloch rotation no longer exist. Anyway, it provides several basic interactions:
1. It uses Clifford tableau as underlying data representation (different from the state representation).
2. It can be constructed from a tableau or list of operations (`_has_stabilizer_effect_` only). All Clifford gates can be built through \{S, H, CNOT\}, so we can construct any Clifford Gate from the list of operations. We just cannot pre-define it.
3. Decomposing into several basic operations.
4. Get unitary matrix through decomposing (we cannot do this in a reverse way from unitary to Clifford gate :( ).
5. Know how to interact with ActOnCliffordTableauArgs, i.e. it should be able to use with CliffordTableau simulator (Looks like we don't have that in cirq yet? @daxfohl will add that? see quantumlib#4639 and quantumlib#4748.).
This PR is part of efforts for quantumlib#3639. Context: this PR doesn't introduce any new algorithms but the key methods are already implemented in quantumlib#4183 and quantumlib#4096.
…#4748) Fixes quantumlib#4639, ref [tinyurl.com/clifford-simulators-refactor](https://tinyurl.com/clifford-simulators-refactor) This change makes the Clifford simulators more consistent with other simulators. Rather than the gate having all the logic of how to update the simulator state, which seemed out-of-place and limited the ability to reuse in any new kinds of Clifford simulators, this PR moves all the logic to the simulators themselves. It also creates an ABC for Clifford simulator states, allowing reuse of the evolution logic in the base class, such that the subclasses just have to implement the specific gate applicators. Note I *tried* doing this via a new Clifford protocol as well ([link](https://github.com/quantumlib/Cirq/compare/master...daxfohl:clifford3?expand=1)) as we originally discussed, and it works but I didn't like the result. The protocol ended up just duplicating all the information of the gate, in a weird structure, and felt like an extra, unnecessary, hacky wrapper around just using the gate itself. So I prefer the approach in this PR that just uses the gate instances directly, though I'm open to suggestion. @tanujkhattar @viathor (cc @ybc1991)
Add initial Clifford Gate with multiple qubits. Compared with SingleQubitCliffordGate, it has fewer functionalities since we cannot enumerate all of them with PauliGates and several special single qubit properties like Bloch rotation no longer exist. Anyway, it provides several basic interactions:
1. It uses Clifford tableau as underlying data representation (different from the state representation).
2. It can be constructed from a tableau or list of operations (`_has_stabilizer_effect_` only). All Clifford gates can be built through \{S, H, CNOT\}, so we can construct any Clifford Gate from the list of operations. We just cannot pre-define it.
3. Decomposing into several basic operations.
4. Get unitary matrix through decomposing (we cannot do this in a reverse way from unitary to Clifford gate :( ).
5. Know how to interact with ActOnCliffordTableauArgs, i.e. it should be able to use with CliffordTableau simulator (Looks like we don't have that in cirq yet? @daxfohl will add that? see quantumlib#4639 and quantumlib#4748.).
This PR is part of efforts for quantumlib#3639. Context: this PR doesn't introduce any new algorithms but the key methods are already implemented in quantumlib#4183 and quantumlib#4096.
Fixes #4639, ref tinyurl.com/clifford-simulators-refactor
This change makes the Clifford simulators more consistent with other simulators. Rather than the gate having all the logic of how to update the simulator state, which seemed out-of-place and limited the ability to reuse in any new kinds of Clifford simulators, this PR moves all the logic to the simulators themselves.
It also creates an ABC for Clifford simulator states, allowing reuse of the evolution logic in the base class, such that the subclasses just have to implement the specific gate applicators.
Note I tried doing this via a new Clifford protocol as well (link) as we originally discussed, and it works but I didn't like the result. The protocol ended up just duplicating all the information of the gate, in a weird structure, and felt like an extra, unnecessary, hacky wrapper around just using the gate itself. So I prefer the approach in this PR that just uses the gate instances directly, though I'm open to suggestion.
@tanujkhattar @viathor (cc @ybc1991)