Add modulo to _equal_up_to_global_phase_ #6058
Conversation
cirq-core/cirq/ops/eigen_gate.py
Outdated
| @@ -388,7 +388,7 @@ def _equal_up_to_global_phase_(self, other, atol): | |||
| return False | |||
|
|
|||
| period = self_without_phase._period() | |||
| canonical_diff = (exponents[0] - exponents[1]) % period | |||
| canonical_diff = abs((exponents[0] - exponents[1])) % period | |||
There was a problem hiding this comment.
Thank you for taking care of this! The comparison seems to be still failing the #5980 (comment) example for
gate1 = cirq.Z
gate2 = cirq.Z ** (3 - 1e-10)
The exponents are compared at a period=2, but the current formula fails for differences just below like 1.99999.
Please use near_zero_mod to compare the exponents instead.
Also please add unit tests for the exponent values where comparison fails before the fix.
I'd suggest to check [1-eps, 1, 1+eps, 2-eps, 2, 2+eps, 3-eps, 3, 3+eps] where the comparison should return True for exponents close to 1 and 3 and False for the exponents near 2.
There was a problem hiding this comment.
@siddharth-mehta - seems like this issue went off the radar.
Are you OK with me finishing this off or would you prefer to update the code yourself?
| (cirq.Z, cirq.Z ** (1 + 1e-10), True), | ||
| (cirq.Z**2, cirq.Z ** (2 - 1e-10), True), | ||
| (cirq.Z**2, cirq.Z ** (2 + 1e-10), True), | ||
| (cirq.Z, cirq.Z ** (3 - 1e-10), True), |
There was a problem hiding this comment.
nit: Does this test for any behavior different from the 1 - 1e10 case above?
There was a problem hiding this comment.
Yes, the critical part is to verify the exponents are recognized as equivalent w/r to a period if their difference is eps-larger or eps-smaller than the period. This was not right in master and not quite right in the first iteration here.
Taking a second look, it would make more sense to compare (Z**2, Z**(4±eps)) so I am going to update it that way.
Resolves Issue: #5980
Adds a modulo operator to the
_equal_up_to_global_phase_function