Bring canonicalization of FSimGate and PhasedFSimGate angles b/w [-pi, pi) #4576
Conversation
tanujkhattar
added
the
BREAKING CHANGE
cirq-core/cirq/ops/fsim_gate.py
Outdated
| if protocols.is_parameterized(value): | ||
| return value | ||
| period = 2 * np.pi | ||
| return value - period * np.round(value / period) | ||
| numerator = value + np.pi - 1e-10 |
There was a problem hiding this comment.
We should not be introducing numerical error. Can you think of a better way of canonicalizing into a half-open interval?
There was a problem hiding this comment.
I realized that subtracting the constant is not actually necessary. Modified the code to remove the numerical error and bring the canonicalization in [pi, pi).
This reason this works is because (x + pi) % (2 * pi) - pi will bring everything between [-pi, pi) and I used division and floor instead of built-in modulo for better accuracy and lower error.
tanujkhattar
changed the title
| """Returns True iff param, assumed to be in [-pi, pi], is 0 (mod pi).""" | ||
| return param in (-np.pi, 0.0, np.pi, -sympy.pi, sympy.pi) | ||
| """Returns True iff param, assumed to be in [-pi, pi), is 0 (mod pi).""" | ||
| return param in (0.0, -np.pi, -sympy.pi) |
There was a problem hiding this comment.
Optional: We could add an assert to verify that param is in [-pi, pi). Now that the result of these calculations will be used for equality checks, it's probably better to have the code crash and signal a bug than silently pretend equal things are different.
There was a problem hiding this comment.
The equality checks are done by directly comparing the self.theta etc. angles, and these functions are also called only with the class angles, which are always canonicalized. I could add an assert to the _canonicalize method which ensures that the return value is between [-pi, pi), but the tests already ensure that the method works as expected so I don't think the assert is needed.
…, pi) (quantumlib#4576) * Bring canonicalization of fsim/phasedfsim angles b/w [-pi, pi) * Get rid of numerical error in fsim angles canonicalization
…, pi) (quantumlib#4576) * Bring canonicalization of fsim/phasedfsim angles b/w [-pi, pi) * Get rid of numerical error in fsim angles canonicalization
Fixes #4574
Breaking Change:
FSimGateandPhasedFSimGate's angles would now be canonicalized between[-pi, pi)instead of[-pi, pi]. This means for cases where angles wherepi, the diagrams will now change (now print-piinstead ofpi), and value equality would now return true when compared with angles equal to-pi.