New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make cirq.FREDKIN
gate self-inverse
#6135
Make cirq.FREDKIN
gate self-inverse
#6135
Conversation
@@ -110,6 +110,11 @@ def test_unitary(): | |||
), | |||
atol=1e-8, | |||
) | |||
np.testing.assert_allclose( | |||
cirq.unitary(cirq.FREDKIN**-1), | |||
cirq.linalg.map_eigenvalues(cirq.unitary(cirq.FREDKIN), lambda b: b**-1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need the call to map_eigevalues
:
np.testing.assert_allclose(cirq.unitary(cirq.FREDKIN**-1), cirq.unitary(cirq.FREDKIN), 1e-8)
Mathematical reason is that the eigenvalues of FREDKIN are all cirq.FREDKIN**-1 == cirq.FREDKIN
, before we even call cirq.unitary
. Perhaps that is a better (or additional) thing to assert (though perhaps not inside test_unitary
, not sure what a better suited test is). Any potential extra assert is up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree we should add a assert cirq.FREDKIN ** -1 == cirq.FREDKIN
. The reason I used this approach for the test here is to assert that the unitary matrix itself is self inverse, and not the object. Self inverse nature of the unitary matrix can be figured out using the math, but here the goal was to test that math.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The simplest way to do this is
u = cirq.unitary(cirq.FREDKIN)
np.testing.assert_allclose(u @ u, np.eye(8))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplified the test for self-inverse as recommended. Also, note that assert cirq.FREDKIN ** -1 == cirq.FREDKIN
is already covered in my original PR under the equality test eq.add_equality_group(cirq.CSWAP(a, b, c), cirq.FREDKIN(a, b, c), cirq.FREDKIN(a, b, c) ** -1)
. Merging now.
The
CSWAP
/FREDKIN
gate is self inverse but currentlycirq.FREDKIN ** -1
returns a_InverseCompositeGate(cirq.FREDKIN)
. This PR adds a__pow__
method toCSwapGate
so thatcirq.FREDKIN ** -1
returns acirq.FREDKIN
.