Speed up clifford simulator by short circuiting on known gates #2919
Conversation
|
|
||
| def _apply_Y(self, qubit: int): | ||
| self.tableau._Y(qubit) | ||
| self.ch_form._Y(qubit) |
There was a problem hiding this comment.
I think we've decided that Clifford gates will define their effect on the tableau/CH form. IOW, this should be defined in the respective gates, not special cased here. Instead, here you should be invoking a protocol. The protocol would check whether a gate defines an effect on tableau/CH form and if so would use it for efficiency. If not, it would fall back to computing the effect.
This is also consistent with _apply_unitary_.
(Also, I wonder whether it makes sense to keep both the tableau and CH form.)
There was a problem hiding this comment.
Thanks for the reminder. This is stated in #2423. Also, with respect to your last statement; here is my take on it: #2423 (comment)
There was a problem hiding this comment.
_clifford_tableau_ hasn't been created yet so we can block this and wait or merge and have it be fixed once _clifford_tableau_ has been created. I'm down for either.
There was a problem hiding this comment.
Given that using the current implementation (which was recently added) is significantly slowing down the simulation, I'd vote for letting this PR go through.
I will work on implementing the _clifford_tableau_ method and protocol and clean this up ASAP.
There was a problem hiding this comment.
@viathor are you okay with letting this optimization merge in and I can clean up later.
| self.ch_form._Z(qubit) | ||
|
|
||
| def _apply_Y(self, qubit: int): | ||
| self.tableau._Y(qubit) |
There was a problem hiding this comment.
I think it would be cleaner if _apply_Gate methods accept the actual Qid and internally resolve that to self.qubit_map[Qid] instead of having the caller do that since the qubit map is an internal implementation detail of the state. (thinking in terms of eventually moving the tableau application to the Gate definitions)
| def _apply_Y(self, qubit: int): | ||
| self.tableau._Y(qubit) | ||
| self.ch_form._Y(qubit) | ||
|
|
There was a problem hiding this comment.
Also add and use _apply_CZ and _apply_CNOT for completeness.
There was a problem hiding this comment.
I'm uncomfortable with the pattern of adding these methods for more and more gates. See comment about method dispatch above.
What is preventing us from implementing the equivalent methods in the gate classes instead?
There was a problem hiding this comment.
@viathor Nothing really. Ideally we can take this to conclusion by removing the _GATE methods on the tableau and CH form too and let individual gates modify the internal states of tableau and the CH form. It will be more involved but I can make that change.
Do we want to have two separate protocols: apply_clifford_tableau and apply_stabilizer_ch_form or combine them into one update_clifford_state? (names subject to change)
@mpharrigan Feel free to ignore the initial suggestion in this comment.
| def _apply_Y(self, qubit: int): | ||
| self.tableau._Y(qubit) | ||
| self.ch_form._Y(qubit) | ||
|
|
There was a problem hiding this comment.
I'm uncomfortable with the pattern of adding these methods for more and more gates. See comment about method dispatch above.
What is preventing us from implementing the equivalent methods in the gate classes instead?
| @@ -331,7 +330,21 @@ def apply_unitary(self, op: 'cirq.Operation'): | |||
|
|
|||
| def apply_single_qubit_unitary(self, op: 'cirq.Operation'): | |||
| qubit = self.qubit_map[op.qubits[0]] | |||
| # Handle H natively as optimization. | |||
| if op.gate == cirq.I: | |||
There was a problem hiding this comment.
We're basically implementing method dispatch here. We should not be doing this because python supports it natively.
Strilanc
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
CirqBot
removed
automerge
|
Thanks all. I agree that a more principled solution is needed; but wanted to share this small change which helped my work and doesn't make anything worse :) |
This brings run-time down from 1.298s to 0.265s on a particular circuit I was investigating