-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix inconsistent Pauli gate multiplication #7603
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
base: main
Are you sure you want to change the base?
Conversation
Enhanced PauliString multiplication methods to handle GateOperations that can be interpreted as Pauli strings (e.g. X**2) by using the existing _try_interpret_as_pauli_string function. Addressed part of quantumlib#7588, for the PauliString * GateOperation cases, e.g. `x*x*x**2`
... to handle operations with single-item pauli expansions using PauliString. Addressed the GateOperation * GateOperation cases in quantumlib#7588.
d9f4abe
to
fc2abac
Compare
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.
Two quick comments
...so that the PauliString construction logic in quantumlib#7588 is defined in a single function "_try_interpret_as_pauli_string".
85d3bc4
to
0247926
Compare
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.
Looking good. Couple more comments.
1. Remove cases that have been handled in _mul_with_qubits 2. Remove try catch block in _try_interpret_as_pauli_string (quantumlib#7588)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7603 +/- ##
========================================
Coverage 97.50% 97.50%
========================================
Files 1101 1103 +2
Lines 99396 99714 +318
========================================
+ Hits 96913 97228 +315
- Misses 2483 2486 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Nice, a couple more things I'd like you to try. Then also add the test cases from the linked issue to pauli_string_tests.
bd84d42
to
0579e62
Compare
... by trying to first convert the operation to be multiplied to a PauliString. With this change, the multiplication dunder function in pauli operation could be removed.
0579e62
to
6ea83a0
Compare
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.
This is looking great. I think the high level structure works well, just a couple nits and I think we'll be ready to wrap it up.
Relax the type check from GateOperation to Operation.
...since it is unused now.
0837a45
to
a15cb41
Compare
Given that _try_interpret_as_pauli_string also handles PauliString after relaxing the type check to Operation, remove the PauliString type check here.
a52bd84
to
7a516c9
Compare
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.
One more nit and I think this looks good to go. Great work!
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.
LGTM. Fixes the problem, and makes code more flexible and removes the need for special cases.
cc @pavoljuhas or @NoureldinYosri for maintainer review.
@@ -614,13 +614,13 @@ def _as_pauli_mask(val: Iterable[cirq.PAULI_GATE_LIKE] | np.ndarray) -> np.ndarr | |||
|
|||
|
|||
def _attempt_value_to_pauli_index(v: cirq.Operation) -> tuple[int, int] | None: |
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.
@ToastCheng Note that this could easily be expanded now to return a list[tuple[int, int]]
if v has multiple qubits, and allow DensePauliString to multiply and commute with parity gates etc on LineQubits too. I think that would be a separate PR, but would be a straightforward followup.
that can be interpreted as Pauli strings (X2, Y2, Z**2, etc.)
operations with single-item pauli expansions using PauliString
This fixes #7588.