Fix commutation analysis to group gates into blocks of pairwise commuting gates

[alexanderivrii]

Summary

This fixes the unsound optimization problem described in #8020: CommutativeAnalysis groups nodes into blocks so that in each block every gate is only required to commute with the immediately preceding and the immediately following gates, while CommutativeCancellation cancels gates assuming that all the gates in a block pairwise commute. This PR changes CommutativeAnalysis to group nodes into blocks of pairwise commuting gates.

As an example, when the list of gates on a given wire is ['X', 'I', 'H', 'I', 'X'], the old behavior grouped all of the gates into a single block and unsoundly canceled pairs of X-gates, while the new behavior groups gates into 3 blocks ['X', 'I'], ['H', 'I'], ['X'] and avoids this unsound cancellation.

Details and comments

This unsound optimization problem was described by @Randl more than a year ago in PR #8056. Looking at that PR again, I now see that it actually suggests a fix that is almost identical to the one in this PR: when deciding whether to put a new gate into the current block we should check whether this new gate commutes with every gate in the block rather than only with the very last gate of the block. I guess the reason that #8056 was closed rather than merged is that it originally only added the failing test but not the fix. @Randl, may I put you as a co-author to this PR?

As CommutativeCancellation is used by default when transpiling with optimization levels 2 or 3, it makes sense to finally fix this. However, the updated commutativity check is now worst-case quadratic and in theory may lead to some slow-downs. Though intuitively I don't think that we should see slow-downs: first, as soon as a gate does not commute with some gate in the given block, we end the block and start a new one; second, the underlying CommutationChecker hashes the results and avoids expensive matrix multiplications. But to be completely sure, @mtreinish, what kind of benchmarking would you recommend me to run?

Fixes #10928

[qiskit-bot]

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

• @Qiskit/terra-core

[coveralls]

Pull Request Test Coverage Report for Build 5848445316

• 5 of 5 (100.0%) changed or added relevant lines in 1 file are covered.
• 29 unchanged lines in 3 files lost coverage.
• Overall coverage decreased (-0.03%) to 87.229%

---

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/expr.rs	1	93.76%
crates/qasm2/src/lex.rs	4	91.14%
crates/qasm2/src/parse.rs	24	95.74%

Totals
Change from base Build 5837476817:	-0.03%
Covered Lines:	74272
Relevant Lines:	85146

---

💛 - Coveralls

[Randl]

Nice! Of course, feel free to add me.

What about missed optimization from #8020 (comment)? If I understand correctly, your fix misses it too? I think a separate issue for optimizing it should be added then.

[alexanderivrii]

Thanks @Randl. I have added you as a co-author (your name appears in commit d4fa355, and when the PR is merged, all the commits will be squashed together).

Regarding potentially missed optimizations, Qiskit now has quite a bunch of single-qubit optimization passes, and I would expect that the specific optimization you mention is already handled by some other pass. Actually, could you please describe your general optimization idea?

[Randl]

The idea is that checking all previous gates may be too strict, or, more generally, there grouping is too weak, as discussed in this comment #8056 (comment)

e.g., if C commutes with A and B but A and B do not commute, then [A,C,B,C^(-1)] will be split as [A, C], [B, C^(-1)] and won't be optimized to [A, B].

cc @jakelishman @1ucian0

[alexanderivrii]

e.g., if C commutes with A and B but A and B do not commute, then [A,C,B,C^(-1)] will be split as [A, C], [B, C^(-1)] and won't be optimized to [A, B].

Oh, thanks, I see what you mean now. At some point I have added a pass CommutativeInverseCancellation that does exactly what you wrote:

for each operator go back as far as you can and try to see if it cancels with something,

see https://qiskit.org/documentation/_modules/qiskit/transpiler/passes/optimization/commutative_inverse_cancellation.html.

As @jakelishman commented, we also have the DAGDependency data structure exploting commutation between individual gates, however I did not find it suitable for the CommutativeInverseCancellation pass above.

[alexanderivrii]

Update: I have removed the long post with old flaky benchmarking results. I have rerun the benchmarks again and there is no noticeable change in performance. For posterity here is the asv benchmarking command used.

1) asv continuous -E conda-py3.10 --show-stderr --split --no-only-change main fix-commutation-analysis --bench "Level.*time"

[coveralls]

Pull Request Test Coverage Report for Build 6547337139

• 5 of 5 (100.0%) changed or added relevant lines in 1 file are covered.
• 13 unchanged lines in 3 files lost coverage.
• Overall coverage decreased (-0.009%) to 87.067%

---

Files with Coverage Reduction	New Missed Lines	%
qiskit/pulse/library/waveform.py	3	93.75%
crates/qasm2/src/lex.rs	4	90.66%
crates/qasm2/src/parse.rs	6	97.6%

Totals
Change from base Build 6541699793:	-0.009%
Covered Lines:	74024
Relevant Lines:	85020

---

💛 - Coveralls

[alexanderivrii]

I don't understand the reason for the error I am seeing after updating the branch (and I don't think it's related to the changes here). I will update the branch and see if the error still persists.

My previous concern was whether the change in behavior (checking that a gate commutes with all of the gates in the current block vs. only the last gate in the block) increases runtime. I have locally rerun the asv benchmarking above and all of the benchmarks have stayed the same.

[jakelishman]

That test failure ought to be unrelated - it's similar to something we've seen before. The default clock available on Windows is lower resolution than it is on macOS or Linux, and so the algorithms tests that "some non-zero execution time was reported" are more likely to fail on Windows if we do our job quickly: see #8577, for example.

[jlapeyre]

This looks good to me.

Possible follow-up work regarding the worst-case quadratic problem mentioned above: For very long blocks of commuting gates, it might be worth maintaining a set of unique gates in the block as you build it. So gates already in this set would be inserted into the block rather than checking commutation explicitly.

[jlapeyre]

Looks good.