Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions crates/transpiler/src/commutation_checker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,24 @@ impl CommutationChecker {
_ => (),
};

// Special handling for commutativity of two pauli product measurements
if let (
OperationRef::PauliProductMeasurement(ppm1),
OperationRef::PauliProductMeasurement(ppm2),
) = (op1, op2)
{
if cargs1 == cargs2 {
// If both PPMs write to the same classical bit, it's generally incorrect to interchange them.
// So we return true only if both PPMs are identical.
return Ok(qargs1 == qargs2 && ppm1 == ppm2);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From your PR comment, you were worried about the case (ZX, [0, 1]) and (XZ, [1, 0]). The simplest algorithm would be to "canonicalise" both PPMs into sorted order in terms of the qargs and then do an elementwise comparison. That's (naively, at least) $\mathcal O(n \lg n)$ for $n$ qubits.

I wouldn't say you have to do that in this PR, though.

Fwiw, if the two PPMs are identical, then any optimisation that applies to one necessarily applies to the other too, so on the face of it, it doesn't really matter whether you report them as commuting or not, but it would matter if you're then intending to group things into mutually commuting terms; if you report them as falsely non-commuting then you might fail to commute another rotation through both measures and to cancel with something on the other side.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point on collecting mutually commuting sets of gates - I have not thought of that.

I have fixed the canonicalization problem in 6323efa in the spirit of the existing code: constructing SparseObservable generators and checking whether they are equal. In the process I realized that try_pauli_generator ignores the phase of a pauli product measurement instruction, which is probably fine in general, but not in this particular case -- as this would lead to incorrectly commuting PPM("XYZ") and PPM("-XYZ").

Note that constructing a SparseObservable instead of just retrieving the Pauli is an overkill, and I have a follow-up PR #15815 that significantly improves upon commuting PPRs/PPMs among themselves (including canonicalization, but temporary having the same problems that are being fixed here).

} else {
// PPMs write to different classical bits, and they commute if and only if their pauli generators do.
let size = qargs1.iter().chain(qargs2.iter()).max().unwrap().0 + 1;
let pauli1 = try_pauli_generator(op1, qargs1, size).expect("extracting sparse observable generator from pauli product measurement in infallible");
let pauli2 = try_pauli_generator(op2, qargs2, size).expect("extracting sparse observable generator from pauli product measurement in infallible");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not necessarily for this PR, but these expects are an indication that the API is wrong here. There should probably be a function

observable_generator_from_ppm(ppm: &PauliProductMeasurement, qubits: &[Qubit], num_qubits: u32) -> SparseObservable {}

that you can call here. (and try_pauli_generator would use that, etc)

It's generally not a great idea to go from a specific type (&PauliProductMeasurement here) in your pattern guard back up to a more generic type (OperationRef) - it often leads to this kind of code that has to make assumptions about validity, when the language gives us tools to have the compiler check it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was thinking about this, especially after reviewing your PRs :smiling. I have implemented this suggestion in 6323efa, mostly because I also needed to return the phase of the pauli in addition to the sparse observable.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This else block basically just decays to the code that's right below it anyway, especially with the current use of the generic try_pauli_generator, and you don't actually need to extract the ppm objects to check the cargs (bugs in the test suite notwithstanding), so you could potentially replace this whole if let with

if num_cargs1 > 0 && num_cargs2 > 0 && cargs1 == cargs2 {
    return Ok(op1 == op2);
}

and avoid all the rest of the extra code?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I have removed the else branch in 6323efa.

return Ok(pauli1.commutes(&pauli2, tol));
}
}
// Handle commutations between Pauli-based gates among themselves, and with standard gates
// TODO Support trivial commutations of standard gates with identities in the Paulis
let size = qargs1.iter().chain(qargs2.iter()).max().unwrap().0 + 1;
Expand Down
8 changes: 8 additions & 0 deletions releasenotes/notes/fix-ppm-commutation-ddcde0bf8b20a9f8.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
fixes:
- |
Fixed commutativity check between two :class:`.PauliProductMeasurement` instructions
in the case that both instructions measure to the same classical bit. In this case,
the later measurement overwrites the result of the earlier measurement, and consequently,
interchanging the two measurement instructions inside the quantum circuit is generally
invalid.
28 changes: 24 additions & 4 deletions test/python/circuit/test_commutation_checker.py
Original file line number Diff line number Diff line change
Expand Up @@ -525,10 +525,11 @@ def test_pauli_based_gates(self, gate_type):
for p1, q1, p2, q2, expected in cases:
if p1 == "I" and gate_type == "measure":
continue # PPM doesn't support all-identity gates
c1, c2 = ([0], [1]) if gate_type == "measure" else ([], [])

gate1 = build_pauli_gate(p1, gate_type)
gate2 = build_pauli_gate(p2, gate_type)
self.assertEqual(expected, scc.commute(gate1, q1, [], gate2, q2, []))
self.assertEqual(expected, scc.commute(gate1, q1, c1, gate2, q2, c2))

@data(
("pauli", "measure"),
Expand All @@ -544,10 +545,26 @@ def test_mix_pauli_gates(self, gate_type1, gate_type2):
]

for p1, q1, p2, q2, expected in cases:
c1 = [0] if gate_type1 == "measure" else []
c2 = [1] if gate_type2 == "measure" else []

gate1 = build_pauli_gate(p1, gate_type1)
gate2 = build_pauli_gate(p2, gate_type2)

with self.subTest(p1=p1, p2=p2):
self.assertEqual(expected, scc.commute(gate1, q1, [], gate2, q2, []))
self.assertEqual(expected, scc.commute(gate1, q1, c1, gate2, q2, c2))

def test_ppms_with_same_clbit(self):
"""Test commutativity of two Pauli product measurements with the same clbit."""
ppm1 = build_pauli_gate("XXII", "measure")
ppm2 = build_pauli_gate("IIZZ", "measure")

with self.subTest("different paulis"):
self.assertFalse(scc.commute(ppm1, [0, 1, 2, 3], [0], ppm2, [0, 1, 2, 3], [0]))
with self.subTest("same paulis, different qubits"):
self.assertFalse(scc.commute(ppm1, [0, 1, 2, 3], [0], ppm1, [1, 0, 2, 3], [0]))
with self.subTest("same paulis and qubits"):
self.assertTrue(scc.commute(ppm1, [0, 1, 2, 3], [0], ppm1, [0, 1, 2, 3], [0]))

def test_pauli_evolution_sums(self):
"""Test PauliEvolutionGate commutations for operators that are sums of Paulis."""
Expand Down Expand Up @@ -585,7 +602,8 @@ def test_pauli_and_standard_gate(self, pauli_type):
"""Test Pauli-based gates and standard gate commutations are efficiently supported."""
# 40-qubit Pauli gate with following terms: X: 0-9, Y: 10-19, Z: 20-29, I: 30-39
pauli = 10 * "I" + 10 * "Z" + 10 * "Y" + 10 * "X"
pauli_indices = list(range(len(pauli)))
pauli_qubits = list(range(len(pauli)))
pauli_clbits = [0] if pauli_type == "measure" else []
Comment thread
alexanderivrii marked this conversation as resolved.
pauli_gate = build_pauli_gate(pauli, pauli_type)

# Test cases in the format: (gate, indices, commutes)
Expand All @@ -607,7 +625,9 @@ def test_pauli_and_standard_gate(self, pauli_type):

for std_gate, indices, expected in cases:
with self.subTest(std_gate=std_gate, indices=indices):
commutes = scc.commute(pauli_gate, pauli_indices, [], std_gate, indices, [])
commutes = scc.commute(
pauli_gate, pauli_qubits, pauli_clbits, std_gate, indices, []
)
self.assertEqual(expected, commutes)


Expand Down