cirq.MutablePauliString supports identity gates but cirq.PauliString doesn't, which leads to inconsistencies

[tanujkhattar]

Description of the issue
cirq.MutablePauliString seems to support identity gates (for example: when passed in pauli_int_dict parameter). The type annotations on on methods like .values() and .items() also have a Union[Pauli, IdentityGate].

This is bad because cirq.PauliString does not support identity gates and hence this leads to inconsistencies between the mutable vs the non-mutable version.

How to reproduce the issue

>>> q = cirq.LineQubit.range(3)
>>> mps = cirq.MutablePauliString(pauli_int_dict={q[0] : 0, q[1] : 1, q[2] : 2})
>>> [*mps.values()]
[cirq.I, cirq.X, cirq.Y]
>>> [*eval(repr(mps)).values()]
[cirq.X, cirq.Y]

Cirq version
0.15dev

[tanujkhattar]

Also, I see that cirq.MutablePauliString is not an operation; and uses the underlying .frozen() version (i.e. a PauliString) when comparing two instances of MutablePauliString.

[Strilanc]

Operations are hashable, mutable things aren't hashable, so a mutable thing can't be an operation.

No strong opinion on keeping or removing the identities but my default expectation would be that they were removed. Do some tests fail if they're removed?

[dabacon]

I don't remember why we had a mutable version of PauliString. Was it just convenience?

[tanujkhattar]

Also, since it's not an operation, can we make it a private class?

[vtomole]

@tanujkhattar It used to be private: #3299

[vtomole]

@dabacon

Was it just convenience?

Looks like it was for performance reasons: Add private _MutablePauliString type for aggregating products efficiently - #2232