Fix post-oxidization change in ConsolidateBlocks behavior

[ElePT]

Summary

Fixes the behavioral difference introduced by #13368 that caused #13438.

The original implementation of the pass used to set the ConsolidateBlocks internal decomposer calling unitary_synthesis._decomposer_2q_from_basis_gates, which, when kak_basis wasn't found, would return None. During the oxidization efforts, unitary_synthesis._decomposer_2q_from_basis_gates was replaced with inline code that didn't contemplate this path (always set a default decomposer), which in the reproducing example lead to a longer output circuit (the gates were collected into a unitary block and then decomposed differently).

Details and comments

Technically not a bugfix because the change wasn't released yet.

Using the example shown in the issue ->

ConsolidateBlocks output dag with 1.2.4:

ConsolidateBlocks output dag with 1.3.0rc1:

ConsolidateBlocks output dag after this fix:

[qiskit-bot]

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

• @Qiskit/terra-core

[mtreinish]

Ah, good catch.it was a change in behavior on my consolidate blocks pr not unitary synthesis. I remember changing that logic at the time and thinking it wouldn't impact anything. I was coming at it from the mindset that consolidate blocks is too likely to skip consolidation when it shouldn't.

Realistically we should be relying on the xx decomposer with the rzz in the basis gates list, but there isn't an interface in rust yet to tell us the estimated number of 2q basis gates needed for a unitary, which is why I changed the logic here. Also that is not really a good meteic anyway, just more reason for me to keep working on #13419 and introduce a better approach.

[ElePT]

Changing the "default" to None now left the paths for rzz and rzx gates unaccounted for. That broke the unit test that was fixed in e3124a6. So, questions:

1. any specific reason we had not to have rzz in the list? (because I just re-added it)
2. assuming that we don't want to use the XXDecomposer for rzx. What do we want to have then, None or TwoQubitBasisDecomposer(CXGate())? It currently doesn't break any test but just to confirm.

[coveralls]

Pull Request Test Coverage Report for Build 11898770714

Details

• 4 of 5 (80.0%) changed or added relevant lines in 1 file are covered.
• 4 unchanged lines in 3 files lost coverage.
• Overall coverage increased (+0.01%) to 88.954%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/transpiler/passes/optimization/consolidate_blocks.py	4	5	80.0%

Files with Coverage Reduction	New Missed Lines	%
crates/accelerate/src/unitary_synthesis.rs	1	92.2%
qiskit/transpiler/passes/optimization/consolidate_blocks.py	1	96.36%
crates/qasm2/src/lex.rs	2	92.48%

Totals
Change from base Build 11842392021:	0.01%
Covered Lines:	79420
Relevant Lines:	89282

---

💛 - Coveralls

[ShellyGarion]

why is the angle here fixed to pi/2 ?

[mtreinish]

It's because that's what UnitarySynthesis uses if it's a basis_gates only setup: https://github.com/Qiskit/qiskit/blob/main/qiskit/transpiler/passes/synthesis/unitary_synthesis.py#L98 the idea is that it's trying to match the decomposer used for estimating to what the synthesis pass will use later.

The whole logic between the passes is more than a bit hacky. ConsolidateBlocks tries to guess how many 2q gates will be used to decide whether to consolidate the block into a unitary or not. This is strongly coupled to the behavior of UnitarySynthesis because that's what actually does the synthesis later. But none of this actually takes into account multiple possible decomposers/synthesis methods being supported by a target, or the presence of a non-default plugin being used.

[mtreinish]

FWIW, I did some testing locally with this and in some cases using TwoQubitBasisDecomposer(RXXGate(pi/2)) under estimates what the XXDecomposer estimates, 3 vs 4 gates. This might result in some blocks not getting consolidated which were before, but in practice I find it probably pretty unlikely since it only counts rzz gates when evaluating if the block's gates are more than the expected synthesis output.

[ElePT]

I added a small refactoring on top of the previous changes to make the new implementation hopefully closer to the old one. Now:

• The default decomposer is None when no basis_gates/kak_gates are found (same as OG). This includes the rzz gate from the issue [EDIT: didn't work. Reverted that change]
• If an rxx gate is found, the decomposer will be the TwoQubitBasisDecomposer with a RXX(pi/2) (same as OG)
• If an rzx gate is found, the decomposer will be the TwoQubitBasisDecomposer with a CXGate (same as backup decomposer from OG implementation)

I also added a comment that mentions the previous use of the XXDecomposer for the rzx case for clarity.

[mtreinish]

I'm fine with this it returns the previous state of logic. There might be some subtle differences in what gets consolidated or not with rzx based on differences between XXDecomposer.num_basis_gates and TwoQubitBasisDecomposer's logic, but it should be close enough to the 1.2.x state of the logic that I'm not worried.

I think this whole exercise reinforces for me how we need to improve the logic around block consolidation. #13419 will do this for the optimization loop, but we will need to think what makes the most sense for when we run ConsolidateBlocks prior to layout in the init stage.