Fix: Skip UnitaryGate instances during BasisTranslator

[raynelfss]

Summary

Fixes #15278
Fixes #15733

• This aims to fix a well known bug introduced by Make BasisTranslator rust-native. #14659 in which the AerSimulator is known to define UnitaryGate as part of its target basis. However, BasisTranslator should not include UnitaryGate as part of its calculations, it should instead be handled by UnitarySynthesis.
• To fix this, BasisTranslator skips any UnitaryGate in the circuit by filtering its op_nodes to check for any UnitaryGate instances.

Details and comments

The only question left is whether we should still have the panic condition in name_to_packed_operation because this check is done on a name basis rather than instance.

[qiskit-bot]

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

• @Qiskit/terra-core

[raynelfss]

The only question left is whether we should still have the panic condition in name_to_packed_operation because this check is done on a name basis rather than instance.

I'd like to bring this up again, in case it is a genuine concern. Since most of this fix might be superseded by #15684, we won't need to even go through Python in the future.

[mtreinish]

Where specifically is the panic? We shouldn't be panicking in general on the name unitary, that's something that will be used a lot. In general we shouldn't be panicking unless the underlying data structures are completely invalid (as in something corrupted them).

[raynelfss]

Where specifically is the panic? We shouldn't be panicking in general on the name unitary, that's something that will be used a lot.

I agree with this @mtreinish. I mistakenly added this in the past believing that there was no way we'd end up ever retrieving or querying a UnitaryGate or a gate named "unitary" from an equivalence library.

The panic happens specifically during the compose_transforms method where we would introduce a placeholder gate with the provided name into a dag. However, since we moved most of the infrastructure to rust, to remove ourselves from python, we would create rust native versions of gates. The original implementation included a variant for unitary but after further discussion we agreed that it wouldn't make sense for a unitary to end up there anyway, so I mistakenly made it into an unreachable!() call.

Ideally, it would be best to throw away this panic condition in favor of just going through Python instead since #15684 will modify this to perform all tasks from rust.

[ShellyGarion]

Is this PR still relevant? I checked both issues against the main branch (locally), and I didn't get any failure (there was no panic)?

This is the test added to my main branch:

    @unittest.skipUnless(HAS_AER, "Aer backend required for simulation")
    def test_basis_with_unitary_basis_aer(self):
        """Test that a circuit with a unitary gate in its target basis gets
        skipped by the BasisTranslator"""
        from qiskit_aer import AerSimulator
        from qiskit_aer import QasmSimulator 

        qr = QuantumRegister(3, "q")
        cr = ClassicalRegister(3, "c")
        qc = QuantumCircuit(qr, cr)

        qc.cx(0, 1)
        qc.h(0)
        qc.cx(0, 1)
        qc.sx(0)
        qc.dcx(0, 2)

        backend = AerSimulator()
        pm = generate_preset_pass_manager(
            optimization_level=2, backend=backend, seed_transpiler=134
        )
        transpiled = pm.run(qc)
        print (transpiled)
        self.assertEqual(Operator(qc), Operator(transpiled))

        n_wires = 4
        qc = QuantumCircuit(n_wires, 1)

        qc.cx(2, 3)
        qc.ry(0.1, 3)
        qc.cx(2, 3)
        qc.ry(-0.1, 3)
        qc.cx(2, 3)
        qc.crz(0.3, 3, 1)  
        qc.s(3)
        qc.barrier()
        qc.measure(0, 0)

        backend = QasmSimulator() 
        transpiled_qc = transpile(qc, backend=backend)
        print (transpiled_qc)

and these are the output circuits:

     ┌──────────┐     ┌───┐
q_0: ┤0         ├──■──┤ X ├
     │  Unitary │  │  └─┬─┘
q_1: ┤1         ├──┼────┼──
     └──────────┘┌─┴─┐  │  
q_2: ────────────┤ X ├──■──
                 └───┘     
c: 3/══════════════════════
                           
                                   ░ ┌─┐
q_0: ──────────────────────────────░─┤M├
     ┌──────────┐                  ░ └╥┘
q_1: ┤ Rz(0.15) ├─■────────────────░──╫─
     ├──────────┤ │                ░  ║ 
q_2: ┤0         ├─┼────────────────░──╫─
     │  Unitary │ │ZZ(-0.15) ┌───┐ ░  ║ 
q_3: ┤1         ├─■──────────┤ S ├─░──╫─
     └──────────┘            └───┘ ░  ║ 
c: 1/═════════════════════════════════╩═
                                      0 
ok

[alexanderivrii]

This and the other issues seem to have been fixed by Ray's PR #15684 with the support of OperationRef::CustomOperation(_) in basis_translator/mod.rs. @raynelfss , does this make sense to you, or you think there might be other problems as well?

[ShellyGarion]

If these issues were fixed in a former PR, it would be better to add a release note as well as a test for this.

[raynelfss]

I've decided to close this PR as the panic condition no longer exists and this pass may be removed in the future.

[ShellyGarion]

I've decided to close this PR as the panic condition no longer exists and this pass may be removed in the future.

so we should also close the two issues?