add_control: check for predefined gates by type instead of name

[willhbang]

Summary

• Fixes Make add_control identify known definitions by type not name #3540

Details and comments

• Usage of Unroller also seems to depend on name, but testing for type there seemed to break a lot of tests. Unroller's usage of name isn't affected by user input (within the context of add_control), so maybe it's moot for this PR.

[ewinston]

control also needs checks for u1, u3, and cx.

[edited] May be this isn't necessary until Unroller does the same but would be within the goal of this PR if it doesn't break anything.

[willhbang]

@ewinston To make sure I understand, we should check that the operation is not u1, u3, or cx, so we can simply apply the control before we go through the trouble of unrolling into those bases?
(i.e. those cases would go here? https://github.com/Qiskit/qiskit-terra/blob/025f6f9e73572a4aa608ac6e90bb1d7bc557e51f/qiskit/circuit/add_control.py#L116)

[willhbang]

@ewinston looks like a change in #3739, specifically here:
https://github.com/Qiskit/qiskit-terra/blob/025f6f9e73572a4aa608ac6e90bb1d7bc557e51f/qiskit/circuit/controlledgate.py#L85

is causing the definition get in this test to throw with
TypeError: can only concatenate list (not "NoneType") to list
because the gate generated from this circuit has no definition.

Is that ok/expected, since the gate here is degenerate? I originally added no operations to the test circuit, since it wouldn't be relevant to the testing the name bug. If so i'll add some arbitrary gate to the circuit.

[ewinston]

I think in this special case, where the controlled gate has an empty _definition, the definition method of ControlledGate should perhaps also return the same. Could you add an

else:
    return self._definition

to ControlledGate.definition after the elif clause to see if that resolves it?

[willhbang]

I was going to add a case for cx below, but realized it's already handled here for arbitrary controls. Is it worth doing the same for the other gate checks below?

[ewinston]

This might be good to do for the rotation gates as well. It would be interesting to check if it reduces gate count.

[willhbang]

@ewinston gave this a shot in the commit below, but it's breaking add_control for RZ and CRZ gates. Not sure where to start investigating... any ideas where to dig further?

[ewinston]

I'm not sure why this breaks but if you remove the or condition in _operation_has_base-gate the issue seems to resolve. Then it's just isinstance(operation, gate) like in original logic.

[willhbang]

@ewinston Besides RZ, it looks like checking for the base gate to avoid unrolling did reduce gate count, at least for the tests I ran: https://www.diffchecker.com/DmorhS8x

from qiskit import QuantumCircuit, QuantumRegister
from qiskit.extensions.standard import XGate, RXGate, RYGate, RZGate, U1Gate, U3Gate
import numpy as np


angles = list(np.divide(pi, list(range(1, 5)))) + [0]
num_controls = list(range(3, 6))

for gate in (RXGate, RYGate, RZGate, U1Gate):
    print(gate)
    for num in num_controls:
        for theta in angles:
            mcgate = gate(theta).control(num)
            print(len(mcgate.definition))

print(U3Gate)
for num in num_controls:
    for theta in angles:
        for phi in angles:
            for lamb in angles:
                mcgate = U3Gate(theta, phi, lamb).control(num)
                print(len(mcgate.definition))