Proposed fix for #3529 (Functionality decoupling for _strat_commutes_… #3548
Conversation
…_commutes_from_operation)
| @@ -698,7 +698,10 @@ def zip_paulis( | |||
| ) -> Iterator[Tuple[pauli_gates.Pauli, pauli_gates.Pauli]]: | |||
| return (paulis for qubit, paulis in self.zip_items(other)) | |||
|
|
|||
| def _commutes_(self, other: Any, atol: float) -> Union[bool, NotImplementedType, None]: | |||
| def _commutes_( | |||
There was a problem hiding this comment.
this seems unrelated - can you revert this formatting change?
There was a problem hiding this comment.
I see an error when I'm doing this
Run check/mypy
cirq/ops/pauli_string.py:701: error: Signature of "_commutes_" incompatible with supertype "Operation"
Found 1 error in 1 file (checked 745 source files)
There was a problem hiding this comment.
So I adjusted the
- arguments as in Operation super
- function definition length, since it was too long (I had an error from the git checks)
There was a problem hiding this comment.
I see what the problem is. You moved the * argument with the _strat_commutes_from_operationmethod. I think we don't need that for _commutes_. Can you remove it from all the touched _commutes_ methods?
There was a problem hiding this comment.
in fact there seems to be some inconsistency around this in the codebase:
Alright, let's keep the * in front of atol, I'll open an issue to cleanup this inconsistency.
|
@balopat please see my answers above, also I adjusted the code |
|
Hi @balopat, I'm back to Cirq. Could you please check the changes? |
| @@ -167,8 +167,14 @@ def _unitary_(self) -> Union[np.ndarray, NotImplementedType]: | |||
| return getter() | |||
| return NotImplemented | |||
|
|
|||
| def _commutes_(self, other: Any, atol: float) -> Union[bool, NotImplementedType, None]: | |||
| return self.gate._commutes_on_qids_(self.qubits, other, atol=atol) | |||
| def _commutes_( | |||
There was a problem hiding this comment.
We should be consistent within the PR at least and add the * parameter before atol.
There was a problem hiding this comment.
OK, I'll merge this, and then we can fix the inconsistency in #3695
balopat
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
|
Automerge cancelled: A required status check is not present. Missing statuses: ['cla/google'] |
CirqBot
removed
automerge
balopat
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
CirqBot
removed
automerge
protocols/commutes_protocol.pytoops/raw_types.py:Operator_commute_functionality to check if the parent class commutes)