quantumlib / Cirq

A Python framework for creating, editing, and invoking Noisy Intermediate Scale Quantum (NISQ) circuits.
Apache License 2.0
4.28k stars 1.02k forks source link

Rename RotPGates to PowPGate and make RotPGates follow standard conventions #865

Closed babbush closed 6 years ago

babbush commented 6 years ago

I find the output of the following code extremely surprising. In every textbook the rotation around X gate is always defined as R_X(theta) = e^{-i theta X / 2}. But that's not true in Cirq. Not only does RotXGate differ from this by a global phase, but the function is 2pi periodic instead of 4pi periodic. The last part is extremely confusing for those of us that try to use these gates for quantum simulation (we are thinking about evolving for certain amounts of time, rather than the geometry of the Bloch sphere). I was trying to make a controlled gate that corresponded to controlled evolution under X and because of these strange global phase conventions my circuit wasn't working. This is such unexpected behavior that I suggest we consider it a bug.

import numpy
import scipy
import cirq

class MyRotXGate(cirq.KnownMatrix, cirq.Gate):
    def __init__(self, rads):
        self.rads = rads   
    def matrix(self):
        X = numpy.array([[0, 1], [1, 0]])
        unitary = scipy.linalg.expm(-1.j * self.rads * X / 2.)
        return unitary

for angle in numpy.linspace(0, 3 * numpy.pi, 10):
    A = MyGate(rads=angle)
    B = cirq.RotXGate(rads=angle)
    print('My gate')
    print(A.matrix())
    print('Cirq gate')
    print(B.matrix())
    print()
bryano commented 6 years ago

I think this is actually reasonable behavior. In cirq, RotXGate(rads=θ) corresponds to Xθ/π. In some sense, the difference is between thinking of X as observable vs. as a unitary. Maybe renaming RotXGate (and its siblings) to something like PauliXGate would make this clearer.

It's important to distinguish between the two different issues here: the global phase and the factor of 2.

The global phase issue is not just for the Pauli rotation gates; see #816. Personally, the decision to really embrace that global phase is physically irrelevant seems reasonable, especially given the availability of the helper function cirq.linalg.predicates.allclose_up_to_global_phase.

The factor of two is more inconvenient. One reasonable solution would be to have a second gate RX that includes the factor of 2; that would make it usable by people familiar with the physicists' convention without having to change any existing code.

maffoo commented 6 years ago

To be clear, the factor of 2 difference in periodicity and the global phase are really the same thing; if you look at expectation values like np.abs(np.dot(A, [1, 0]))**2 vs np.abs(np.dot(B, [1, 0]))**2 for @babbush's A and B you'll see that the two gates "rotate" the state at the same rate. The periodicity difference is only in the global phase which is physically unobservable. Of course, if you consider this as a controlled operation, then that phase is not "global" so it starts to matter. But we don't have any standard way in cirq of taking a generic gate and turning it into a controlled operation; @babbush, how are you trying to do that?

kevinsung commented 6 years ago

@maffoo There is now ControlledGate, which is what @babbush is using. It constructs the matrix in the expected way (adding an identity block in the top left).

babbush commented 6 years ago

What @kevinsung said is correct. @bryano yes, I support renaming things. The definition that Cirq is using is inconsistent with the definition of a rotation gate in any textbook.

vtomole commented 6 years ago

@bryano I prefer keeping the global phase as I've spent a lot of time trying to figure out what was going on with it when I started using Cirq. I'm sure one or two other users have too. I agree with @babbush that this is a bug. A second RX gate will make things more confusing than the "factor of 2" issue in my opinion.

I'm in favor of renaming RotXGate to PauliXGate and defining RotXGate with its textbook definition.

bryano commented 6 years ago

As @maffoo pointed out, there actually is no factor of 2 issue, and the difference does matter when controlling. That makes it seem like renaming the current rotation gates and adding new ones with the right phases is the right way to go. The biggest question is whether @Strilanc is okay with that.

kevinsung commented 6 years ago

In my experience, Controlled-X always refers to the gate

[1 0 0 0]
[0 1 0 0]
[0 0 0 1]
[0 0 1 0]

which is not Controlled-exp(-i pi/2 X); that would be

[1 0 0 0]
[0 1 0 0]
[0 0 0 -i]
[0 0 -i 0]

Similarly, Controlled-Z always refers to

[1 0 0 0]
[0 1 0 0]
[0 0 1 0]
[0 0 0 -1]

and not Controlled-exp(-i pi/2 Z), which would be

[1 0 0 0]
[0 1 0 0]
[0 0 -i 0]
[0 0 0 i]

So I think Cirq's current convention leads to the correct, least surprising, controlled gates.

babbush commented 6 years ago

We aren't talking about a controlled X. We are talking about a controlled rotation around the X axis, which is certainly not what Cirq is giving us.

On Sat, Aug 18, 2018, 00:11 Kevin J. Sung notifications@github.com wrote:

In my experience, Controlled-X always refers to the gate

[1 0 0 0] [0 1 0 0] [0 0 0 1] [0 0 1 0]

which is not Controlled-exp(-i pi/2 X); that would be

[1 0 0 0] [0 1 0 0] [0 0 0 -i] [0 0 -i 0]

Similarly, Controlled-Z always refers to

[1 0 0 0] [0 1 0 0] [0 0 1 0] [0 0 0 -1]

and not Controlled-exp(-i pi/2 Z), which would be

[1 0 0 0] [0 1 0 0] [0 0 -i 0] [0 0 0 i]

So I think Cirq's current convention leads to the correct, least surprising, controlled gates.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/quantumlib/Cirq/issues/865#issuecomment-414037960, or mute the thread https://github.com/notifications/unsubscribe-auth/ANlTf7Kz4TPUBaT9AjrhvqrxIqipa8zDks5uR74egaJpZM4WCPwf .

kevinsung commented 6 years ago

I'm not sure what you mean. ControlledGate(RotXGate(rads=theta)) is certainly a controlled rotation around the X axis.

babbush commented 6 years ago

A controlled X rotation should be |0><0| I + |1><1| R_x(theta). In any textbook R_x(theta) = e^{-i theta X / 2}. But not in Cirq. That's a problem.

kevinsung commented 6 years ago

I'm just saying that by your logic, a controlled X rotation by pi radians would give the gate

[1 0 0 0]
[0 1 0 0]
[0 0 0 -i]
[0 0 -i 0]

which is in my opinion a more surprising result than

[1 0 0 0]
[0 1 0 0]
[0 0 0 1]
[0 0 1 0]

Of course I can only speak for myself. I also would find it surprising that a controlled X rotation by 2pi radians would give

[1 0 0 0]
[0 1 0 0]
[0 0 -1 0]
[0 0 0 -1]

rather than the identity. But maybe that's just because we're using the word "rotation".

One solution is to have ExpXGate which would have @babbush 's preferred phase convention, with the understanding that the "Rot" prefix is reserved for the Bloch sphere interpretation of rotations, which have a period of 2pi. I have a feeling this is exactly how ExpZGate came to be (and getting rid of it is being discussed at #702).

Strilanc commented 6 years ago

I've been thinking a bit about how to make these two conventions mesh together in a more seamless way (because it does seem like sometimes you want one e.g. CZ = diag(1,1,1,e^it) and sometimes you want the other e.g. simulation or desire to stay within SU).

babbush commented 6 years ago

I'm in favor of pretty much all of these suggestions Craig.

maffoo commented 6 years ago

@Strilanc, I also like your suggestions. A few comments:

  • Is there some way for us to make equivalence-up-to-global-phase more of a first class citizen in the library? E.g. should cirq.Z == cirq.iZ?

This seems like a bad idea to me. Having explicit helpers to check equality up to global phase (as we do now for testing) seems much better than overriding equality.

  • We could add a global phase property to RotXGate and friends. It would likely require dropping the concept of the gate being period.

This seems potentially interesting; for example, gates that have a matrix could know their SU(N) representation as well as a phase (I don't think we should call it a "global phase" because it is not global as soon as you put it inside a controlled gate, which is a confusion I think we should avoid). When you ask for the matrix you'd get the product of these two, but you could also ask for the decomposed form and this might be useful in some cases, e.g. when checking equality up to global phase. But it's not immediately clear to me that this would be worth the trouble.

  • If we drop the concept of gates being periodic, it fixes the fact that (U3)(1/3) is not equal to U in several cases (e.g. 240 degrees 3 canonicalizes to 120 degrees, then 120 degrees /3 = 60 degrees; instead we would always say 240deg 3 = 720deg). Do we ever actually use this concept? How bad is it to have cirq.X**3 be matrix-equivalence to cirq.X, and yet they disagree about what their square root is?

We might want to drop "canonicalization" of arguments in constructors, and only do that when we really need to. For example, when compiling to hardware gates we actually care about applying the smallest "equivalent" rotation (e.g. a 90 degree rotation instead of a 450 = 360 + 90 degree rotation) and since these hardware rotations are never "controlled" the extra phase really is global to the entire state and can be ignored. The non-canonicalized gates are still manifestly periodic if you look at their matrices, so we don't lose much there. And if you need to add back in optimization rules that can simplify things based on known periodicities, then at least those have to be invoked explicitly when you do a circuit optimization, rather than during gate construction.

  • ControlledGate should probably have a global-phase-on-sub-gate property.

Related to my comments above about being careful with "global": the name of this property sounds to me like an oxymoron :-)

dabacon commented 6 years ago

I agree we should change. We've gotten this feedback from lots of different folks now (not just silly Dave back when were painting the shed!) I also hadn't realized that we had shifted the gates away from the gates as defined in the proto file, sorry I should have yelled when we did that. I thought that at least our single qubit gates were in SU.

I like Craig's suggestions, but I worry that having Rx and RotX is very confusing and no one is going to remember which is which. In thinking about this I think we should be guided by the fact that X**s is the non-standard notation in the quantum computing community. I do think it is useful, but I think we should error on making the support for at a gate feature level the one that has the name that is very non-standard.

Is there some way for us to make equivalence-up-to-global-phase more of a first class citizen in the library? E.g. should cirq.Z == cirq.iZ?

I agree with @maffoo that we should probably not do this equality.

Also +1 to @maffoo s suggestion that we not call it a global phase. What should it be called? Really the relevant object here is the determinant of the matrix.

Strilanc commented 6 years ago

It can't be the determinant; det(-X) = det(X). We would get N-fold ambiguity for NxN matrices.

Strilanc commented 6 years ago

Another idea I had was to have the pauli gates play double-duty as gates and as Hamiltonians. So cirq.X * 5 would be a valid thing, but no longer a gate since it's not unitary, and you could say stuff like cirq.exp(1j * cirq.X * np.pi/2). cirq.exp would perhaps require anti-Hermitian arguments, and return a gate.

On the other hand, that's getting quite loose with what exactly the Pauli types are and it could be confusing. Or maybe people would just use them the way they expected and it would all work out. It would at least be a solid answer to "How do I get that thing I expected": "use the notation you're more used to"!

dabacon commented 6 years ago

The definition of SU(d) is that det X = 1. https://en.wikipedia.org/wiki/Special_unitary_group . Not sure what you mean about ambiguity for NxN matrices. Further the determinat is the part that multiplies out separately: AB = det(A)det(B) (A/det(A)) (B/det(B)) = det(AB) (A/det(A)) (B/det(B)).

I think it is a very bad idea to mix objects that are downstairs in quantum computing (unitaries, i.e. elements of the Lie Group) from upstairs (operators that can be expotentiated, i.e. elements of the Lie Algebra). "They are just matrices" is true, but hides a lot of the differences in things you can do with these that make sense in quantum computing.

dabacon commented 6 years ago

It occurs to me that a good name for the gate that is currently RotXGate is PowXGate, since its essential use comes about when thinking about taking the power of a gate.

EhsanZ4t1qbit commented 6 years ago

Same as @babbush I have spent a lot of times to figure out that the definition of the RotXGate is not consistent with what we are used to in the quantum information text book. Would be nice if the Rotation gates can be defined consistently based on the known definition. I agree with @dabacon to call the currents RotXGate as PowXGate to clear the confusion.

dabacon commented 6 years ago

OK renamed this bug as I think we've decided to make the breaking change of renaming Rot gates Pow gates, and change Rot gates to new convention. This is a breaking change and one that can cause considerable pain since it will change the exiting Rot gates in a subtle way.