-
Notifications
You must be signed in to change notification settings - Fork 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
Add functionalty for determining whether pairs of moments commute #6679
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6679 +/- ##
=======================================
Coverage 97.82% 97.82%
=======================================
Files 1072 1072
Lines 92047 92075 +28
=======================================
+ Hits 90045 90074 +29
+ Misses 2002 2001 -1 ☔ View full report in Codecov by Sentry. |
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.
sorry of the late review
# Decompose both moments onto each disjoint set of qubits and | ||
# check for commutation using the unitary representation | ||
if all( | ||
definitely_commutes( |
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.
would this code raise an error if the operations are not unitary?
@@ -669,25 +676,92 @@ def _commutes_(self, other: Any, *, atol: float = 1e-8) -> Union[bool, NotImplem | |||
|
|||
Returns: | |||
True: The Moment and Operation commute OR they don't have shared | |||
quibits. | |||
qubits. |
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.
can you fix the formatting?
# Decompose into disjoint overlapping sets of qubits | ||
qubit_subsets = [list(op.qubits) for op in self.operations + other.operations] | ||
disjoint_set = DisjointSet(itertools.chain.from_iterable(qubit_subsets)) | ||
for subset in qubit_subsets: |
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.
what happens if a qubit appears in both moments in operations with different qubits?
so in m1: op1 acting on (q1, q0) in m2 op2 acting on q3, q4, q0)
if isinstance(other, ops.Operation): | ||
# If an Operation is provided, convert this to a Moment consisting only | ||
# of the given Operation | ||
return self._commutes_(Moment(other), atol=atol) |
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.
why not preserve the old code? ... perhaps in a def _commutes_with_op
@@ -693,6 +693,18 @@ def test_commutes(): | |||
assert not cirq.commutes(moment, cirq.X(c)) | |||
|
|||
|
|||
def test_commutes_multiqubit_gates(): | |||
a = cirq.NamedQubit('a') |
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 test case is very simple and doesn't cover the more complex cases ... see above
Currently it is not possible to determine if pairs of moments can commute which may be useful for when collections of operations commute as a group but not individually
For instance a pair of single qubit Z gates commute with a two qubit RXX gate together but not separately.
This PR fixes this challenge by decomposing two moments into operations that act on disjoint sets of qubits and then testing whether their unitary representations on these sets of qubits commute.
Closes #6659