Add commutes protocol #1853

bryano · 2019-07-19T21:11:21Z

Minimal version so far. Wanted to get some feedback.

Some thoughts:

A lot of issues seemed to be caused by values_equality being the name of both a module and a method, so we should probably change at least one of those for commutes.
Maybe _commutes_with_ for the magic method? (I just went with convention in the initial issue for now.)
I changed cirq.commutes to be the protocol, which delegates to cirq.linalg.commutes in the relevant cases. Shouldn't be a problem, but may cause confusion.
Some classes already define a commute_with method. Should I add some underscores to that or add a new method that delegates to the existing one?
If passed a Gate and GateOperation whose gate commutes with the Gate, what should be returned?

c-poole · 2019-07-22T18:35:33Z

cirq/protocols/commutes.py

+    return NotImplemented
+
+
+def _strat_commutes_from_matrix(left_val: Any, right_val: Any, atol: Union[int, float] = 1e-8


This doesn't appear to take into consideration the possibility that left_val and right_val are operations that act on different qubits.

As an extension to this train of thought, I think that if one is a Gate_Operation and the other is a Gate then the protocol should treat it as a strictest case scenario where the Gate acts on the same qubits as the Gate_Operation.

The more I think about it, the less I think this general strategy is worth it. Makes more sense to have all the logic in the _commutes_with_ method of the various objects. For example, qubit order using the unitary protocol is tricky with circuits.

c-poole · 2019-07-22T18:36:24Z

cirq/protocols/commutes.py

+        ) -> Union[NotImplementedType, bool]:
+    """Attempts to determine commutativity via the objects' _commutes_ method."""
+    for a, b in [(left_val, right_val), (right_val, left_val)]:
+        getter = getattr(a, '_commutes_', None)


+1 on changing to _commutes_with_ and refactoring classes that already have this semantics.

mpharrigan · 2019-07-25T22:37:49Z

cirq/protocols/commutes.py

+
+    This is determined by any one of the following techniques:
+
+    - Either value has a `_commutes_` method that returns something besides


note that left_val is considered first

mpharrigan · 2019-07-25T22:39:10Z

cirq/protocols/commutes.py

+
+    - Either value has a `_commutes_` method that returns something besides
+      NotImplemented. The return value is whatever the method returned.
+    - Each value is either a matrix or defines one via the unitary protocol.


split this into two cases? 1. is a matrix 2. defines one via unitary protocol

Obsoleted (delegation to unitary moved to Gate._commutes_with_)

mpharrigan · 2019-07-25T22:40:38Z

cirq/protocols/commutes.py

+    This is determined by any one of the following techniques:
+
+    - Either value has a `_commutes_` method that returns something besides
+      NotImplemented. The return value is whatever the method returned.


"whatever the method returned" -> "the boolean result from this function" or something to emphasize that you can't return whatever you want in _commutes_

mpharrigan · 2019-07-25T22:42:19Z

cirq/protocols/commutes.py

+        result = strat(left_val, right_val, atol)
+        if result is not NotImplemented:
+            return result
+    return False


is this a sensible default? Why not raise a NotImplementedError or return NotImplemented all the way up to the caller? cirq.unitary doesn't return the identity matrix if it can't figure out the unitary representation of an object :)

I imagine this being used for circuit reordering, where what you want to know whether two things definitely commute. (Note that this is the documented behavior.)

Do you have an example where false negatives would be problematic?

mpharrigan · 2019-07-25T22:43:17Z

cirq/protocols/commutes.py

+        getter = getattr(a, '_commutes_', None)
+        if getter is None:
+            continue
+        val = getter(right_val, atol)


this should be b

mpharrigan · 2019-07-25T22:47:04Z

Some classes already define a commute_with method. Should I add some underscores to that or add a new method that delegates to the existing one?

I'd advocate for adding underscores, keeping the old signature which dispatches to the underscore version with a deprecation warning for ~1 release cycle

bryano · 2019-08-07T05:55:29Z

Some classes already define a commute_with method. Should I add some underscores to that or add a new method that delegates to the existing one?

I'd advocate for adding underscores, keeping the old signature which dispatches to the underscore version with a deprecation warning for ~1 release cycle

Done

bryano · 2019-08-07T05:57:30Z

Ready for re-review.

bryano · 2019-10-02T00:12:36Z

What should commutes(A, B) mean when A and B are both gates?

(1) They have the same qid_shape and commute when applied to the same qudits; indeterminate when different qid_shape.

(2) They commute for all possible qubits.

For example CNOT commutes with itself under (1) but not (2), while Z and ZZ commute under (2) but not (1).

Strilanc · 2019-10-02T22:18:11Z

Good question. I think I like (1) best. Specifically:

if qid_shape(a) != qid_shape(b):
    raise ValueError("Different sized gates.")
qs = cirq.LineQid(i, s) for i, s in enumerate(qid_shape(a))
return commutes(a(*qs), b(*qs))

cduck · 2019-10-02T23:09:39Z

@Strilanc This is what LineQid.for_qid_shape is for:

qs = cirq.LineQid.for_qid_shape(qid_shape)

adds Gate._commutes_on_qids_

return value

and coverage

bryano · 2019-12-09T04:29:13Z

@Strilanc PTAL

CirqBot · 2019-12-16T22:24:45Z

Automerge cancelled: A status check is failing.

vtomole · 2019-12-16T22:39:43Z

@Strilanc We can't merge anything until #2648 is merged.

adds commutes protocol

b535a52

googlebot added the cla: yes Makes googlebot stop complaining. label Jul 19, 2019

Merge branch 'master' into commute

ac0c8df

bryano requested review from dabacon, Strilanc and c-poole July 19, 2019 21:11

Bryan A. O'Gorman added 2 commits July 19, 2019 14:12

actually added new files

4efe605

Merge branch 'commute' of https://github.com/bryano/Cirq into commute

741176d

bryano requested a review from mpharrigan July 19, 2019 21:13

c-poole suggested changes Jul 22, 2019

View reviewed changes

mpharrigan reviewed Jul 25, 2019

View reviewed changes

Bryan A. O'Gorman added 3 commits August 6, 2019 18:18

Merge branch 'master' into commute

259998d

finished commutes protocol

6c17875

Merge branch 'master' into commute

faa7603

bryano changed the title ~~[WIP, Fixes #1125] adds commutes protocol~~ [Fixes #1125] adds commutes protocol Aug 7, 2019

Bryan A. O'Gorman and others added 12 commits August 6, 2019 23:04

formatting

c4721c1

Merge branch 'master' into commute

9a9fa40

Merge branch 'master' into commute

6b72adb

updates commutes_with for SwapPermutationGate and GateOperation

8fb1389

Merge branch 'commute' of https://github.com/bryano/Cirq into commute

6ecc8f7

Merge branch 'master' into commute

a921816

Merge branch 'master' into commute

f890598

adds definitely_commutes

0da5f56

Merge branch 'commute' of https://github.com/bryano/Cirq into commute

79cc7fa

Merge branch 'master' into commute

3eefa6a

Merge branch 'master' into commute

bf48a44

Merge branch 'master' into commute

42c06e7

formatting and lint

ed33c9f

Merge branch 'master' into commute

caa1769

Bryan A. O'Gorman and others added 9 commits December 7, 2019 15:30

Merge branch 'master' into commute

3ca25f2

alphabetization

510caa7

raises error on comparison of Gate and GateOperation

d01f565

adds Gate._commutes_on_qids_

adds cirq.testing.assert_commutes_magic_method_consistent_with_unitaries

0d733e3

Merge branch 'master' into commute

e885f0c

Incorporates distinction between None/NotImplemented in _commutes_

3565122

return value

Merge branch 'master' into commute

863ecbc

adds cirq.ZPowGate._commutes_on_qids_

c348a0c

and coverage

Merge branch 'commute' of https://github.com/bryano/Cirq into commute

afe9fc5

Merge branch 'master' into commute

2111b69

Strilanc approved these changes Dec 16, 2019

View reviewed changes

Strilanc added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Dec 16, 2019

Merge branch 'master' into commute

1113b2a

CirqBot removed the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Dec 16, 2019

Strilanc changed the title ~~[Fixes #1125] adds commutes protocol~~ Add commutes protocol Dec 17, 2019

Merge branch 'master' into commute

e36fef4

Strilanc added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Dec 17, 2019

CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Dec 17, 2019

Merge branch 'master' into commute

097329e

CirqBot merged commit ed6188b into quantumlib:master Dec 17, 2019

CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Dec 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add commutes protocol #1853

Add commutes protocol #1853

bryano commented Jul 19, 2019 •

edited by Strilanc

c-poole Jul 22, 2019

c-poole Jul 22, 2019

bryano Jul 23, 2019

c-poole Jul 22, 2019

bryano Aug 7, 2019

mpharrigan Jul 25, 2019

bryano Aug 7, 2019

mpharrigan Jul 25, 2019

bryano Aug 7, 2019

mpharrigan Jul 25, 2019

bryano Aug 7, 2019

mpharrigan Jul 25, 2019

bryano Aug 7, 2019

mpharrigan Jul 25, 2019

bryano Aug 7, 2019 •

edited

mpharrigan commented Jul 25, 2019

bryano commented Aug 7, 2019

bryano commented Aug 7, 2019

bryano commented Oct 2, 2019

Strilanc commented Oct 2, 2019

cduck commented Oct 2, 2019

bryano commented Dec 9, 2019

CirqBot commented Dec 16, 2019

vtomole commented Dec 16, 2019

		return NotImplemented


		def _strat_commutes_from_matrix(left_val: Any, right_val: Any, atol: Union[int, float] = 1e-8


		This is determined by any one of the following techniques:

		- Either value has a `_commutes_` method that returns something besides

Add commutes protocol #1853

Add commutes protocol #1853

Conversation

bryano commented Jul 19, 2019 • edited by Strilanc

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryano Aug 7, 2019 • edited

Choose a reason for hiding this comment

mpharrigan commented Jul 25, 2019

bryano commented Aug 7, 2019

bryano commented Aug 7, 2019

bryano commented Oct 2, 2019

Strilanc commented Oct 2, 2019

cduck commented Oct 2, 2019

bryano commented Dec 9, 2019

CirqBot commented Dec 16, 2019

vtomole commented Dec 16, 2019

bryano commented Jul 19, 2019 •

edited by Strilanc

bryano Aug 7, 2019 •

edited