Test that circuit built from intransitive non-commutative doesn't wrongly cancels

[Randl]

Summary

Following #8020 I think that current implementation of CommutationAnalysis produces unsound optimizations. Adding test that verifies it.

Details and comments

The simple circuit consists of Hadamard gate surrounded by X gates. Adding identity between non-commuting operators results in cancellation of X gates. In real life, though, similar cases should be very unlikely.

[qiskit-bot]

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

• @Qiskit/terra-core

[CLAassistant]

All committers have signed the CLA.

[jakelishman]

Thanks for the test case and the interest in contributing to Qiskit! Unfortunately, we can't merge a failing test to the main branch, even if the buggy behaviour is already in Terra. Our CI and branch-protection rules won't let us, and if we overrode them, it would prevent every other bit of orthogonal work because every pull request would then see this test failure and be forbidden from merging.

It's good to have this test in mind, thanks, but we can't actually put it in the suite until there's a fix that makes it pass that goes in at the same time.

[Randl]

@jakelishman should i try to hotfix it or you prefer to wait for a good general fix?

[Randl]

Ideally I think we should for each operator store the earliest place in DAG where it can be moved rather than storing the set of commuting operators. This should be stronger than just splitting DAG in set of commuting operators. I have an example in mind but need to formalize it

[jakelishman]

@1ucian0 is the owner of this #8020, and I don't know what state he's in with that, so I'll have to defer to him on what the best course of action here is.

[Randl]

Here is a simple example of what can't be optimized with current approach (I won't add it for same reasons):

    def test_non_pairwise_commutative_circuit1(self):
        """Test simple circuit where we can simplify though operators not commute pairwise on 1 qubit"""
        circ = QuantumCircuit(1)

        circ.x(0)
        circ.i(0)
        circ.z(0)
        circ.i(0)

        passmanager = PassManager()
        passmanager.append(CommutativeCancellation())
        ccirc = passmanager.run(circ)

        expected = QuantumCircuit(1)
        expected.x(0)
        expected.z(0)

        self.assertEqual(Operator(circ), Operator(ccirc))
        self.assertEqual(ccirc, expected)

In general if we have A-B-C-D, D commutes with C, B commutes with A, but C doesn't commute with A; and B can be simplified with D, we'll fail since our sets will be [A,B] and [C,D] so we won't try to simplify B and D.

[jakelishman]

There can be several candidates for the same "earliest" spot with commutation relations. For example, if I have an order [A, B, C] where A commutes with both B and C, but B and C don't commute, then there are several possible valid orders:

• A, B, C
• B, A, C
• B, C, A

The number of choices scales superlinearly (I'm not sure what exactly the scaling is), with the number of operators considered, so we can't realistically consider them all at a large scale.

This is an effect of commutation relations being only a partial order, not a total order. An algorithm based on sequencing the operators assumes that there's some canonical total ordering that will always put cancellable gates next to each other, but as far as we know (it feels like it could be proven) there's no way of defining such an order.

This is what things like Qiskit's DAGDependency are looking to solve; rather than being based on a total ordering, they represent the partial ordering of the commutation relations (like a Hasse diagram). Our algorithms on that structure aren't necessarily the most efficient, but the data structure is a more accurate collection of the relations - as you note, the current form of CommutationAnalysis is broken. If you're interested, you can read more about the DAGDependency class in this paper (it's called "canonical form" there) or the paper that's based on. I think you might find it interesting - it looks like you're working towards some of the same ideas.

[Randl]

Nice! I was so focused on trying to solve a problem in front of me that didn't think that there is prior work on it. I'll should go and read some papers.

What I had in my head is something like: "for each operator go back as far as you can and try to see if it cancels with something". This should be quadratic but possibly misses some more complicated optimizations (I have to admit I haven't fully understood the current optimization procedure).

[Randl]

The hotfix I applied just checks commutation with all the operators in current set instead of the last one, which is in spirit of original optimization I think

[1ucian0]

Hi @Randl Thanks a lot for taking a look at this! I think your commutation cancellation bug is a perfect example for #8056. It seems to me that the real fix for #8056 is using DAGDependency. Let's talk on Slack or Discord if you want to exchange possible solutions.