PauliStringPhasorGate does not round trip via operation and gate method.

[dabacon]

Description of the issue

Following #2626 it was discussed that instead of always producing GateOperations, one should instead check that gate(*qubits).gate == gate. However this fails for PauliStringPhasorGate:

gate = cirq.PauliStringPhasorGate(cirq.DensePauliString('XYZI', coefficient=(1+0j)), exponent_neg=0.2, exponent_pos=0.1)
op = gate.on(*cirq.LineQubit.range(4))
print(repr(op.gate))

prints

cirq.PauliStringPhasorGate(cirq.DensePauliString('XYZ', coefficient=(1+0j)), exponent_neg=0.2, exponent_pos=0.1)

The issues seems to be that DensePauliString#on drops the I.

dense = cirq.DensePauliString('XYZI', coefficient=(1+0j))`
print(repr(dense.on(*cirq.LineQubit.range(4))))

prints

(cirq.X(cirq.LineQubit(0))*cirq.Y(cirq.LineQubit(1))*cirq.Z(cirq.LineQubit(2)))

Cirq version
v0.15dev

[daxfohl]

Ick, I'd say if this isn't trivial, then ignore my comment in #2626 and go for the gold. Having everything that has a gate be a plain GateOperation (i.e. not a subclass of GateOperation either, just a GateOperation containing the gate) would be a nice improvement.

[dstrain115]

@dabacon Now that #5354 is merged, can we close this issue? (Or at least take it off of before-1.0)

[dabacon]

We can't close this is as it is still broken. Gate round tripping is tested, but there is a carve out for this class.

[tanujkhattar]

PauliStringPhasorGate fails roundtrip because the underlying cirq.DensePauliString itself fails the roundtrip; which I think is the bigger issue here.

>>> gate = cirq.DensePauliString('XIY')
>>> q = cirq.LineQubit.range(3)
>>> gate.on(*q).gate == gate
False

This happens because dense pauli strings preserve the identity but pauli strings drop the identity.

The fact that gate.on().gate is not consistent for dense pauli strings was also identified in #4270; but it looks like it was only partly fixed (there was an issue with qubit ordering as well earlier); but this fact that identity gate gets dropped was missed in the earlier issue.

We should first try to address this if we want to make all gates consistent, and then pauli string phasor should automatically get fixed.

[daxfohl]

I think the core of the issue is that some code considers identity a Pauli, and some doesn't. Should identity be Pauli or not?

[dabacon]

I think making the change to PauliString is a major change in it's API, and I worry that there are a ton of people depending on this now. Let me do a PR to see how this impacts things and then we need to decide whether to go that route or #5510

[dabacon]

Yeah changing the API of PauliString isn't going to work, that's a major breaking change that goes to the core of how that class is used. PauliString is an operation and has a notion of qubits, and doesn't allow identities. DensePauliString is a gate and has no concepts of qubits. When you use this gate on qubits it turns into a PauliString, dropping the identity components.

Another alternative is to completely deprecated PauliStringPhasorGate and PauliStringPhasor and just create a new gate DensePauliStringPhasorGate (or a better name) that uses the DensePauliString. Trying to retrofit PauliStringPhasor to use DensePauliString would also be possible, but in a pretty challenging way as you'd have to support both during a deprecation cycle.

@tanujkhattar @daxfohl thoughts?

[daxfohl]

I know there are a couple places that use PauliStringPhasor directly, so it might be hard to deprecate (otherwise I'd have probably done exactly that when I created PauliStringPhasorGate). Though IIRC they're all in contrib so maybe it doesn't matter? Otherwise I'm completely on board with that option.

Another option (forgive me if this is dumb), would it make any sense to create a _PauliI class, and make I an instance of that like we do for X, Y, and Z, such that identity can be used in PauliString?