Ensure UnitaryGate's are preserved by transpile if in basis gates

[mtreinish]

Summary

This commit tweaks the preset passmanager definitions to not include the
UnitarySynthesis pass as part of basis translation if the 'unitary' gate
is in the basis gates. Previously in #6124 and #6349 the preset
passmanagers were updated to leverage the user specified unitary
synthesis method instead of implicitly using the default as part of the
unroll 3q and unroll custom definition passes. This however had the
unintended side effect of always synthesizing unitary gates even if it
was in the basis (which is the case on Aer). This commit fixes this by
only running unitary synthesis as part of the basis translation step if
it's not in the basis.

A potential follow-on here is to make the unroll 3q transpiler pass
basis aware (since right now it will implicitly run unitary synthesis
internally) and add a similar logic around the use of the
UnitarySynthesis pass for unrolling 3q or larger gates. The unroll 3q
transpiler stage suffers from this same issue. However, this wasn't
done here because the way 3q unrolling is used is to reduce the gates to
be less than 3 qubits so the layout and routing phase can deal work with
the gates in the circuit would likely cause issues if a unitary gate
larger than 2 qubits was in the circuit being transpiled.

Details and comments

[jakelishman]

I couldn't put a comment on it because it's not in the diffs, but the if translation_method == "synthesis" blocks will also always decompose unitary gates (for example, here's the level1 one): https://github.com/Qiskit/qiskit-terra/blob/f59a1fd4bca6c92683d9487c94db10e6c730ebfd/qiskit/transpiler/preset_passmanagers/level1.py#L193-L215

The first pass won't do it because it's limited to 3+ qubits, but the last pass will. This is probably desired behaviour (else why pass translation_method="synthesis" - it's not the default?), but just bringing up the inconsistency.

[jakelishman]

I think this changes the default behaviour if basis_gates is None - this is the relevant part of UnitarySynthesis.run from immediately before #6124: https://github.com/Qiskit/qiskit-terra/blob/c202e460a7369fde41209c2f41ad1484a3810a31/qiskit/transpiler/passes/synthesis/unitary_synthesis.py#L158-L160

In there, if basis_gates is None then the pass will run and synthesis unitary gates. I would think the old form is possibly what people would expect, but it's a bit of a funny one. If we want to maintain the exact behaviour, then it needs to be if (not basis_gates) or "unitary" not in basis_gates (which also runs it if basis_gates=[]), or perhaps more sensibly, if basis_gates is None or "unitary" not in basis_gates.

Similar comments on every pass level.

[mtreinish]

So, I've been testing this with 0.18.3 to compare the results with this PR:

qc = QuantumCircuit(2)
qc.unitary(random_unitary(4, seed=42), [0, 1])
qc.measure_all()

With result = transpile(qc, basis_gates=None):

0.18.3:

        ┌──────────┐ ░ ┌─┐   
   q_0: ┤0         ├─░─┤M├───
        │  Unitary │ ░ └╥┘┌─┐
   q_1: ┤1         ├─░──╫─┤M├
        └──────────┘ ░  ║ └╥┘
meas: 2/════════════════╩══╩═
                        0  1 

This PR:

        ┌──────────┐ ░ ┌─┐   
   q_0: ┤0         ├─░─┤M├───
        │  Unitary │ ░ └╥┘┌─┐
   q_1: ┤1         ├─░──╫─┤M├
        └──────────┘ ░  ║ └╥┘
meas: 2/════════════════╩══╩═
                        0  1 

With result = transpile(qc, basis_gates=[]):

0.18.3:

qiskit.transpiler.exceptions.TranspilerError: "Unable to map source basis {('barrier', 2), ('u3', 1), ('measure', 1), ('cx', 2)} to target basis {'delay', 'reset', 'snapshot', 'barrier', 'measure'} over library <qiskit.circuit.equivalence.EquivalenceLibrary object at 0x7fbe564d2460>."

This PR:

qiskit.transpiler.exceptions.TranspilerError: "Unable to map source basis {('u3', 1), ('barrier', 2), ('measure', 1), ('cx', 2)} to target basis {'barrier', 'reset', 'measure', 'snapshot', 'delay'} over library <qiskit.circuit.equivalence.EquivalenceLibrary object at 0x7f2dcff347f0>."

So I think the translator method is ok as is. Unless I'm missing something or I need a different example circuit.

[jakelishman]

For the record - the lines I highlighted above aren't the only relevant lines in 0.18.3. For one, the UnitarySynthesis plugin wasn't even run at optimization_level=1 in 0.18.3, because that didn't come til #6349, and for two, even if you force it, there's an implicit skip because _choose_kak_gate(None) would return None, and cause no synthesis to be done.

[mtreinish]

For the synthesis method with this PR it has different behavior compared to 0.18.3:

qc = QuantumCircuit(2)
qc.unitary(random_unitary(4, seed=42), [0, 1])
qc.measure_all()

With result = transpile(qc, basis_gates=['cx', 'u', 'unitary'], translation_method='synthesis'):

0.18.3:

        ┌──────────┐ ░ ┌─┐   
   q_0: ┤0         ├─░─┤M├───
        │  Unitary │ ░ └╥┘┌─┐
   q_1: ┤1         ├─░──╫─┤M├
        └──────────┘ ░  ║ └╥┘
meas: 2/════════════════╩══╩═
                        0  1 

This PR:

global phase: 0.26938
        ┌───────────────────────────┐         ┌─────────────────────┐         »
   q_0: ┤ U(2.3398,-0.43844,1.2137) ├───■─────┤ U(0.52485,-π/2,π/2) ├──────■──»
        ├───────────────────────────┴┐┌─┴─┐┌──┴─────────────────────┴───┐┌─┴─┐»
   q_1: ┤ U(0.58612,-1.4512,0.09315) ├┤ X ├┤ U(0.61426,-1.5731,-3.1397) ├┤ X ├»
        └────────────────────────────┘└───┘└────────────────────────────┘└───┘»
meas: 2/══════════════════════════════════════════════════════════════════════»
                                                                              »
«          ┌──────────────────┐       ┌────────────────────────────┐ ░ ┌─┐   
«   q_0: ──┤ U(0.4452,-π,π/2) ├───■───┤ U(0.97669,-2.2224,-2.6846) ├─░─┤M├───
«        ┌─┴──────────────────┴┐┌─┴─┐┌┴────────────────────────────┤ ░ └╥┘┌─┐
«   q_1: ┤ U(1.5695,1.5695,-π) ├┤ X ├┤ U(0.97271,-2.3231,-0.59794) ├─░──╫─┤M├
«        └─────────────────────┘└───┘└─────────────────────────────┘ ░  ║ └╥┘
«meas: 2/═══════════════════════════════════════════════════════════════╩══╩═
«                                                                       0  1 

So there is something more to fix in the synthesis path. At least it also works for the basis_gates=None case:

With result = transpile(qc, basis_gates=None, translation_method='synthesis'):

0.18.3:

        ┌──────────┐ ░ ┌─┐   
   q_0: ┤0         ├─░─┤M├───
        │  Unitary │ ░ └╥┘┌─┐
   q_1: ┤1         ├─░──╫─┤M├
        └──────────┘ ░  ║ └╥┘
meas: 2/════════════════╩══╩═
                        0  1 

This PR:

        ┌──────────┐ ░ ┌─┐   
   q_0: ┤0         ├─░─┤M├───
        │  Unitary │ ░ └╥┘┌─┐
   q_1: ┤1         ├─░──╫─┤M├
        └──────────┘ ░  ║ └╥┘
meas: 2/════════════════╩══╩═
                        0  1 

With result = transpile(qc, basis_gates=[], translation_method='synthesis'):

0.18.3:

        ┌──────────┐ ░ ┌─┐   
   q_0: ┤0         ├─░─┤M├───
        │  Unitary │ ░ └╥┘┌─┐
   q_1: ┤1         ├─░──╫─┤M├
        └──────────┘ ░  ║ └╥┘
meas: 2/════════════════╩══╩═
                        0  1 

This PR:

        ┌──────────┐ ░ ┌─┐   
   q_0: ┤0         ├─░─┤M├───
        │  Unitary │ ░ └╥┘┌─┐
   q_1: ┤1         ├─░──╫─┤M├
        └──────────┘ ░  ║ └╥┘
meas: 2/════════════════╩══╩═
                        0  1 

I'll add additional tests and come up with a fix for the synthesis path too.

[jakelishman]

I'm ticking this because I'm happy with this PR whether or not we merge the comment I made above, and I'm leaving work for the evening, so I don't want to hold up getting Aer's CI back online if we decide to go ahead without those changes.

If they are made, I'll re-approve again once I've seen them, or someone else can do it for me.