Add EigenGate._equal_up_to_global_phase_ #1840
Conversation
…om/bryano/Cirq into eigengate-equal-up-to-global-phase
cirq/ops/eigen_gate.py
Outdated
| not np.isclose(exponents[0], exponents[1], atol=atol)): | ||
| return False | ||
|
|
||
| self_without_exp_or_phase = self._with_exponent(0) |
There was a problem hiding this comment.
This part confuses me. 1) why does it do a creation of two new objects? 2) what does setting _with_exponent(0) followed by setting global_shift to 0 leave to be different?
There was a problem hiding this comment.
There are EigenGates in OFC that have parameters other than exponent and global_shift.
cirq/ops/eigen_gate.py
Outdated
| not np.isclose(exponents[0], exponents[1], atol=atol)): | ||
| return False | ||
|
|
||
| self_without_exp_or_phase = self._with_exponent(0) |
|
@dabacon ping |
| @@ -322,6 +322,32 @@ def _resolve_parameters_(self: TSelf, param_resolver) -> TSelf: | |||
| return self._with_exponent( | |||
| exponent=param_resolver.value_of(self._exponent)) | |||
|
|
|||
| def _equal_up_to_global_phase_(self, other, atol): | |||
| if not isinstance(other, EigenGate): | |||
There was a problem hiding this comment.
Should be type(self) instead of EigenGate. Otherwise you will end up comparing XPowGate to ZPowGate.
Make sure to test that XPowGate is not approximately equal to ZPowGate up to global phase.
There was a problem hiding this comment.
The method only returns True if the gates without exponents or phases are approximately equal, so the type check can be done there if desired. Certainly there are gates of different types that are equal up to global phase, e.g., X ** 0 and Z ** 0. False negatives are fine (and I think quite common at the moment), but the change you suggest would preclude them from being addressed eventually.
(Note that I added a test for XPowGate and ZPowGate that passes without any changes to the code.)
There was a problem hiding this comment.
I don't think X**0 is approximately equal to Z**0 up to global phase. Their unitaries are equal, but the classes are distinct. If you wanted to compare the unitaries, you'd compare the unitaries.
There was a problem hiding this comment.
You're right that the existing code handles this. I think it's because of the delegating to approx_eq
There was a problem hiding this comment.
Is what "equality" of gates means in Cirq documented anywhere?
| exponents = (self.exponent, other.exponent) | ||
| exponents_is_parameterized = tuple( | ||
| protocols.is_parameterized(e) for e in exponents) | ||
| if (all(exponents_is_parameterized) and exponents[0] != exponents[1]): |
There was a problem hiding this comment.
What is the reason that you needed these checks for? I would have expected that copying with global shift set to 0 and then delegating to approx_eq would have covered the parameterized case.
There was a problem hiding this comment.
This check addressed parameterized exponents only, whereas approx_eq is passed the gates stripped of their exponents.
At one point, I was trying to get it so that gates would be equal up to global phase if their exponents were off by half a period (so a phase of -1), but it got too complicated to be worth it. As it is, the checking of exponents modulo the period might be better put in _approx_eq_.
…om/bryano/Cirq into eigengate-equal-up-to-global-phase
…om/bryano/Cirq into eigengate-equal-up-to-global-phase
|
@Strilanc ping |
Strilanc
changed the title
Strilanc
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
|
Automerge cancelled: Merging is blocked (I don't understand why). |
CirqBot
removed
automerge
Strilanc
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
CirqBot
removed
automerge
No description provided.