Closed perlinm closed 1 month ago
It occurs to me that there are other places in the changed file that check for commutation of two Pauli operators. Perhaps I should just add a _pauli_commutes
method to the file, and use that everywhere within?
EDIT: done
If I work with the commutes protocol, it looks like one neat option is to add something like
def _commutes_(self, other: Any, *, atol: float = 1e-8) -> bool:
return True
to cirq.ops.identity.IdentityGate
. That results in a nearly identical speedup of 2 min --> 40 sec in my original example.
This change seems reasonable because an identity gate commutes with everything, but I'm not sure whether IdentityGate._commutes_
should return NotImplemented
for certain "invalid" types of other
. Thoughts?
What sorts of "invalid" types come to mind?
None come to mind for me 🙂
If you think that addition to IdentityGate is good, I'll just go ahead and add it.
The change to IdentityGate
sounds good to me.
This change seems reasonable because an identity gate commutes with everything, but I'm not sure whether
IdentityGate._commutes_
should returnNotImplemented
for certain "invalid" types ofother
. Thoughts?
Using the IdentityGate option sounds great. The current IdentityGate._commutes_
inherits from the Gate
which returns NotImplemented for non-gate arguments. I think we should keep that behavior, because it can expose bugs in the calling code.
We don't need to redo the qid_shape
check (line 477), because IdentityGate behaves as a plain wire which commutes.
@pavoljuhas great! I think checking for Gate
types covers my worries about invalid inputs.
How are the current proposed changes? I'm not sure what kind of test makes sense for IdentityGate._commutes_
...
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 97.83%. Comparing base (
8d8a6c5
) to head (23a7480
). Report is 1 commits behind head on main.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
My thinking was to ensure that this test calls IdentityGate._commutes_
, rather than deciding that cirq.I
commutes with cirq.X
for independent reasons. I'm happy to change it back though if you prefer.
change it back though if you prefer.
Let's do that. We try to write tests the way an external user would call the API.
Makes sense. Done.
I wrote a custom stabilizer-like simulator in Cirq for this paper (arXiv), which involved commuting Pauli strings through circuit operations. The speed of my simulations turned out to be limited by calls to
protocols.commutes
insideMutablePauliString.inplace_after
. I found that checking conditions for commutation "manually" led to considerable speedups, and figured I could push this change upstream.Long story short: two Pauli operators
P
andQ
commute iff (one ofP
orQ
is the identity operator, orP == Q
).As an example, on my laptop the changes in this PR reduce the runtime of the script below from ~2 minutes to ~39 seconds.