Change MutablePauliString from private to public #3299

Strilanc · 2020-09-03T23:53:28Z

There are several times I've wanted a mutable version of pauli string. In fact, I've previously written an external variant and have seen another person do the same. So I figured it made sense to take the internal version we have and clean it up enough to expose at the top level.

For performance potential, the internal dictionary of mutable pauli string maps to int instead of to gate instances.
Fix pycharm warnings in pauli_string_test.py
Bump qiskit version [encountered load time error in pyton 3.8]
Add frozen/mutable_copy methods to cirq.PauliString
Add after/before methods to cirq.PauliString
Refactor MutablePauliString to construct from any PAULI_STRING_LIKE

- Fix pycharm warnings in pauli_string_test.py - Bump qiskit version [encountered load time error in pyton 3.8] - Add frozen/mutable_copy methods to cirq.PauliString - Add after/before methods to cirq.PauliString - Refactor MutablePauliString to construct from any PAULI_STRING_LIKE

viathor · 2020-09-04T00:02:39Z

Lint, tests and coverage need fixing.

cirq/ops/pauli_string.py

viathor · 2020-09-09T19:05:53Z

cirq/ops/pauli_string.py

+    def __init__(self,
+                 *contents: 'cirq.PAULI_STRING_LIKE',
+                 coefficient: Union[int, float, complex] = 1,
+                 pauli_int_dict: Optional[Dict['cirq.Qid', int]] = None):


It's reasonable for the user to expect to be able to pass a map from qubit objects to gate objects (as e.g. here), but here suddenly they must use ints instead of Paulis. This is made worse by the fact that the mapping from Paulis to ints is hidden in a private constant (and depending on how you think about this, the mapping isn't necessarily obvious or unique, e.g. some users might expect a 2-bit binary number where one bit maps to X and the other to Z). Having qubit-to-int map hidden inside a class is fine, but here we're making it part of the API.

Also, this API is internally inconsistent: we use ints instead of gate objects, but not instead of qubit objects.

Have you checked that using ints rather than gate instances improves performance sufficiently to justify the complications?

I haven't checked the performance, but during multiplication it replaces quite a lot of if-else-if-else-if with a ^ b.

I actually do intend to replace the Qid type with an Any type at a later point, but avoided doing that in this PR because it will also affect PauliString pretty significantly.

This parameter isn't so much for users' convenience as it is an internal shortcut to avoid all conversions and double-checks. That's why its type is so specific. I could prefix it with _ to make it especially clear that it's more of an implementation detail, but for a class driven by performance like this one I think these choices are really part of the API. The dictionary is exposed as a field after all.

WDYT about using the shortcut internally while requiring gate objects in the API? I realize this incurs the cost of isinstance, but perhaps that's acceptable? Alternatively, we could remove that cost by making Pauli._index public, i.e. MutablePauliString.__init__ would accept gates, but then it would convert to ints internally by calling gate.pauli_index or somesuch (failure on non-Pauli gates being a documented and expected behavior).

A large part of the reason that this class exists is because of performance differential. We've had issues in the past w.r.t. equating and hashing custom objects being slow, so I wanted to avoid requiring them.

viathor · 2020-09-09T19:14:56Z

cirq/ops/pauli_string.py

-                            f"{type(contents)}, {repr(contents)}")
+            return NotImplemented
+
+        self.coefficient *= 1j**phase_log_i


This line and phase_log_i = 0 above (l.1221) should go into the first case of the "switch" above.

viathor · 2020-09-09T19:20:10Z

cirq/ops/pauli_string.py

+                self.coefficient *= other.coefficient
+            for qubit, pauli_gate_like in other.items():
+                pauli_int = _pauli_like_to_pauli_int(qubit, pauli_gate_like)
+                phase_log_i += self._imul_atom_helper(qubit, pauli_int, sign)


Accumulating phase in the exponent and then raising i to the resulting total strikes me as unnecessary complication since we can just accumulate phase directly:

self.coefficient *= self._imul_atom_helper(qubit, pauli_int, phase)

This has the following advantages:

Your intermediate values are easier to interpret (phase vs its logarithm).

It allows you to eliminate a variable.

It's immune to numerical error:

In [1]: 1j**104 Out[1]: (1+7.842691359635767e-15j) In [2]: 1j**104 == 1.0 Out[2]: False In [3]: p = 1.0 In [4]: for i in range(1000): ...: if i % 4 == 0: ...: assert p == 1.0 ...: p *= 1j ...:

Note that this requires two changes to _imul_atom_helper: replace sign with phase and change return values -1, 0, +1 to -i, 1.0, i respectively.

I want to keep the amount of math involving complex numbers minimized. The code looks the way it does because that's how it's written in DensePauliString which uses this form because it can be done with a handful of numpy vector operations.

Fixed the numeric precision issue by switching to self.coefficient *= 1j ** (phase_log_i & 3).

Why though? I mean: this is a quantum computing library ;-) If there is any place for complex math anywhere in the world, then this is it! Also, in python it's built-in, so there is really no reason to avoid it.

Look, this does simplify the code. Compare to the suggested alternative:

for qubit, pauli_gate_like in other.items(): pauli_int = _pauli_like_to_pauli_int(qubit, pauli_gate_like) self.coefficient *= self._imul_atom_helper(qubit, pauli_int, phase)

The current code is made more complicated by the presence of exponentiation, bit manipulation, hardcoded constant and unnecessary variable. And on top of that its intermediate values aren't as easy to interpret.

Acknowledged, but I really do prefer this version.

Adding & 3 in the exponent to patch up the numerical issue leads to confusing code. One must consider subtleties of numerical exponentiation to appreciate why the & 3 is even here. And since it appears unnecessary it's easy to imagine that it may be removed in future, inadvertently re-introducing numerical error.

Instead of patching it up, we should just remove the unnecessary complexity - the exponentiation, the bitmasking and the temporary variable phase_log_i - and instead we should have _imul_atom_helper return the phase correction to multiply directly into self.coefficient (rather than having it return one of the logarithms).

It leads to simpler, easily understandable code, doesn't it?

I added a test that will fail if the & 3 is removed.

I disagree that it's less clear. That being said, I am extremely comfortable with bit twiddling so it feels quite natural to me.

Adding ints is faster than multiplying complex numbers. Profiling:

import cirq import time r = cirq.MutablePauliString() r2 = cirq.MutablePauliString() for i in range(1000): r[cirq.LineQubit(i)] = cirq.X r2[cirq.LineQubit(i)] = cirq.Y t0 = time.monotonic() for _ in range(1000): r *= r2 t1 = time.monotonic() print(t1 - t0)

Using ints and bitmasks:

1.9

Using complex multiplication (factors made using 1j**sign):

2.1

Using complex multiplication (factors made using lookup from global R=[1,1j,-1,-1j] using R[sign]):

2.1

(There's some variation, but the ints are consistently lower than the complex.)

It's a minor performance improvement for what (in my mind) is unnecessarily complicated code, but fair enough - now at least I see one minor advantage to working in the logs.

cirq/ops/pauli_string.py

viathor · 2020-09-09T19:55:19Z

cirq/ops/pauli_string.py

+        return self._imul_helper_checkpoint(other, +1)
+
+    def __mul__(self, other: 'cirq.PAULI_STRING_LIKE') -> 'cirq.PauliString':
+        """Multiplies two pauli-stringl-ikes together.


nit: misplaced hyphen

viathor · 2020-09-09T19:57:07Z

cirq/ops/pauli_string.py

+                gate = op.gate
+
+                if isinstance(gate, clifford_gate.SingleQubitCliffordGate):
+                    out = gate.transform(cast(cirq.Pauli, _INT_TO_PAULI[ps[0]]))


FYI (no action needed): Makes me think we should have a type that includes both identity and the Paulis.

Strilanc · 2020-09-10T02:27:28Z

Oh man I've spent at least a few months thinking that XY = iZ instead of -iZ, and these unit tests failing just fixed that.

viathor

Well, if you've been thinking that XY=iZ then you've been right all along! Left-multiplication by X flips the rows, and Y with flipped rows is iZ.

cirq/ops/pauli_string.py

viathor · 2020-09-10T03:28:40Z

cirq/ops/pauli_string.py

+    def __init__(self,
+                 *contents: 'cirq.PAULI_STRING_LIKE',
+                 coefficient: Union[int, float, complex] = 1,
+                 pauli_int_dict: Optional[Dict['cirq.Qid', int]] = None):


WDYT about using the shortcut internally while requiring gate objects in the API? I realize this incurs the cost of isinstance, but perhaps that's acceptable? Alternatively, we could remove that cost by making Pauli._index public, i.e. MutablePauliString.__init__ would accept gates, but then it would convert to ints internally by calling gate.pauli_index or somesuch (failure on non-Pauli gates being a documented and expected behavior).

cirq/ops/pauli_string.py

viathor · 2020-09-10T03:41:09Z

cirq/ops/pauli_string.py

+                self.coefficient *= other.coefficient
+            for qubit, pauli_gate_like in other.items():
+                pauli_int = _pauli_like_to_pauli_int(qubit, pauli_gate_like)
+                phase_log_i += self._imul_atom_helper(qubit, pauli_int, sign)


Why though? I mean: this is a quantum computing library ;-) If there is any place for complex math anywhere in the world, then this is it! Also, in python it's built-in, so there is really no reason to avoid it.

Look, this does simplify the code. Compare to the suggested alternative:

for qubit, pauli_gate_like in other.items(): pauli_int = _pauli_like_to_pauli_int(qubit, pauli_gate_like) self.coefficient *= self._imul_atom_helper(qubit, pauli_int, phase)

The current code is made more complicated by the presence of exponentiation, bit manipulation, hardcoded constant and unnecessary variable. And on top of that its intermediate values aren't as easy to interpret.

…i_string

viathor · 2020-09-11T00:58:45Z

cirq/ops/pauli_string.py

+                self.coefficient *= other.coefficient
+            for qubit, pauli_gate_like in other.items():
+                pauli_int = _pauli_like_to_pauli_int(qubit, pauli_gate_like)
+                phase_log_i += self._imul_atom_helper(qubit, pauli_int, sign)


Adding & 3 in the exponent to patch up the numerical issue leads to confusing code. One must consider subtleties of numerical exponentiation to appreciate why the & 3 is even here. And since it appears unnecessary it's easy to imagine that it may be removed in future, inadvertently re-introducing numerical error.

Instead of patching it up, we should just remove the unnecessary complexity - the exponentiation, the bitmasking and the temporary variable phase_log_i - and instead we should have _imul_atom_helper return the phase correction to multiply directly into self.coefficient (rather than having it return one of the logarithms).

It leads to simpler, easily understandable code, doesn't it?

…i_string

viathor · 2020-09-15T18:31:40Z

cirq/ops/pauli_string.py

+    def __init__(self,
+                 *contents: 'cirq.PAULI_STRING_LIKE',
+                 coefficient: Union[int, float, complex] = 1,
+                 pauli_int_dict: Optional[Dict['cirq.Qid', int]] = None):


viathor · 2020-09-15T18:39:07Z

cirq/ops/pauli_string.py

+                self.coefficient *= other.coefficient
+            for qubit, pauli_gate_like in other.items():
+                pauli_int = _pauli_like_to_pauli_int(qubit, pauli_gate_like)
+                phase_log_i += self._imul_atom_helper(qubit, pauli_int, sign)


It's a minor performance improvement for what (in my mind) is unnecessarily complicated code, but fair enough - now at least I see one minor advantage to working in the logs.

Strilanc requested review from balopat and viathor September 3, 2020 23:53

googlebot added the cla: yes Makes googlebot stop complaining. label Sep 3, 2020

test/lint

30e4d5f

viathor requested changes Sep 9, 2020

View reviewed changes

Adam comments and unit tests

9d5f3d6

Strilanc added 2 commits September 9, 2020 23:47

typecheck

8db7abc

lint

010ee16

viathor reviewed Sep 10, 2020

View reviewed changes

Strilanc added 2 commits September 10, 2020 20:41

format

f216853

Merge branch 'master' of github.com:quantumlib/Cirq into mutable_paul…

fa941c6

…i_string

viathor requested changes Sep 11, 2020

View reviewed changes

Strilanc added 3 commits September 10, 2020 22:04

Fix qasm error message change check

9a575b3

precision test

20444d2

Merge branch 'master' of github.com:quantumlib/Cirq into mutable_paul…

fd1a4e5

…i_string

viathor approved these changes Sep 15, 2020

View reviewed changes

Strilanc merged commit dd3df77 into master Sep 15, 2020

Strilanc deleted the mutable_pauli_string branch September 15, 2020 19:34

vtomole mentioned this pull request Jun 24, 2022

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change MutablePauliString from private to public #3299

Change MutablePauliString from private to public #3299

Strilanc commented Sep 3, 2020

viathor commented Sep 4, 2020

viathor Sep 9, 2020 •

edited

Loading

Strilanc Sep 10, 2020

viathor Sep 10, 2020

Strilanc Sep 10, 2020

viathor Sep 15, 2020

viathor Sep 9, 2020

Strilanc Sep 10, 2020

viathor Sep 9, 2020

Strilanc Sep 10, 2020

viathor Sep 10, 2020 •

edited

Loading

Strilanc Sep 10, 2020

viathor Sep 11, 2020

Strilanc Sep 15, 2020

viathor Sep 15, 2020

viathor Sep 9, 2020

Strilanc Sep 10, 2020

viathor Sep 9, 2020

Strilanc Sep 10, 2020

Strilanc commented Sep 10, 2020

viathor left a comment

viathor Sep 10, 2020

viathor Sep 10, 2020 •

edited

Loading

viathor Sep 11, 2020

viathor Sep 15, 2020

viathor Sep 15, 2020

Change MutablePauliString from private to public #3299

Change MutablePauliString from private to public #3299

Conversation

Strilanc commented Sep 3, 2020

viathor commented Sep 4, 2020

viathor Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viathor Sep 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Strilanc commented Sep 10, 2020

viathor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viathor Sep 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viathor Sep 9, 2020 •

edited

Loading

viathor Sep 10, 2020 •

edited

Loading

viathor Sep 10, 2020 •

edited

Loading