Fixed commutation checking between two Pauli product measurements

[alexanderivrii]

Summary

We have recently realized that checking commutativity between two PauliProductMeasurement instructions (introduced in #15359) is incorrect in the case that both measurements measure to the same classical bit. In this case, the later measurement overwrites the result of the earlier measurement, and thus the order of these instructions in the quantum circuit matters. This PR applies a simple fix of saying that two Pauli product measurement instruction do not commute unless they are completely identical (including pauli, qubits and clbits).

(Updated) This also handles "canonicalization": PPM(XZ, qubits=[0,1], clbits=[0]) is the same as PPM(ZX, qubits=[1,0], clbits=[0]), and so they are now also computed to commute.

Note: the current strategy of constructing sparse observable is in the spirit of the existing code, and I am trying to do minimal changes in order to backport a variant of this to 2.4. I have a followup PR #15815 that greatly increases the efficiency of commuting PPRs/PPMs among themselves (including canonicalization, but temporarily having the same problems as being fixed here).

This also updates commutativity checking tests to pass clbits when checking commutativity between two PPMs.

One other question: do we want to backport the fix to 2.4? If so, we would need to adjust this PR to the commutation checker version before #15488.

AI/LLM disclosure

• I didn't use LLM tooling, or only used it privately.
• I used the following tool to help write this PR description:
• I used the following tool to generate or modify code:

[qiskit-bot]

One or more of the following people are relevant to this code:

• @Qiskit/terra-core

[jakelishman]

It's a known bug with a simple solution, so we probably should backport a fix. If the commutation checker has changed substantially since 2.4 froze, though, then you might need to manually write a different and minimal patch to stable/2.4 instead of trying to auto-backport this one.

[jakelishman]

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.

[alexanderivrii]

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.

[jakelishman]

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.

[alexanderivrii]

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).

[jakelishman]

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?

[alexanderivrii]

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

[alexanderivrii]

I think that we should be able to backport this to stable/2.4 in the current form.

[Cryoris]

We could just add the sign into try_extract_op_from_ppm and avoid duplicating the function -- technically it would be correct to include the sign there and it won't affect any other commutation check.