Fix mutable_pauli_string.inplace_after() and inplace_before() #7507
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7507 +/- ##
==========================================
- Coverage 97.54% 97.50% -0.05%
==========================================
Files 1101 1101
Lines 99430 99411 -19
==========================================
- Hits 96993 96928 -65
- Misses 2437 2483 +46 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hey Nour @NoureldinYosri , could you help take a look at this PR? This is the last pr in fixing #6946. |
cirq-core/cirq/ops/pauli_string.py
Outdated
| flattened_ops = list(op_tree.flatten_to_ops(ops)) | ||
|
|
||
| for op in flattened_ops[::-1]: | ||
| conjugated: cirq.DensePauliString = dense_pauli_string.DensePauliString( |
There was a problem hiding this comment.
maybe conjugates = self.dense(self.qubits) * dense_pauli_string.DensePauliString('I'*len(self.qubits)) should do the trick? ... no need for the logic below
There was a problem hiding this comment.
Good idea! updated.
|
Thanks @NoureldinYosri for the review! Comments applied, could you help take another look? |
cirq-core/cirq/ops/pauli_string.py
Outdated
| pauli = self.get(cast(TKey, q), None) | ||
| match pauli: | ||
| case pauli_gates.X: | ||
| conjugated *= tableau.destabilizers()[qid] |
There was a problem hiding this comment.
I think you wrote this logic before, no? if so can you do some refactoring
There was a problem hiding this comment.
Yeah, some logic can be refactored out, done.
cirq-core/cirq/ops/pauli_string.py
Outdated
| tableau = gate_in_clifford.clifford_tableau.inverse() | ||
|
|
||
| for qid, q in enumerate(op.qubits): | ||
| pauli = self.get(cast(TKey, q), None) |
There was a problem hiding this comment.
why is this cast needed here and below?
There was a problem hiding this comment.
mypy complained: error: Argument 1 to "pop" of "dict" has incompatible type "Qid"; expected "TKey"
babacry
force-pushed
the
inplace_after
branch
3 times, most recently
from
b44fb0c to
c3a6ead
Compare
|
Hey Nour @NoureldinYosri , done addressing the comments, thanks for another round of iteration, could you help take another look? thanks! |
cirq-core/cirq/ops/pauli_string.py
Outdated
| @@ -903,7 +902,7 @@ def conjugated_by(self, clifford: cirq.OP_TREE) -> PauliString: | |||
| The product-of-Paulis $P$ conjugated by the Clifford operation $C$ is | |||
|
|
|||
| $$ | |||
| C^\dagger P C | |||
| C^dagger P C | |||
There was a problem hiding this comment.
why this change? this will break the latex
| C^dagger P C | |
| C^\dagger P C |
There was a problem hiding this comment.
Oops, it was accidentally deleted. Fixed.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Similar to the fix for PauliString.after() #7065, this is the fix for MutablePauliString.
Also deleted the culprit _decompose_into_cliffords helper function that was used by PauliString.after() and MutablePauliString.inplace_after().
Issue: #6946.