Allow unrolling to pauli gates when adding control to a gate

[faisaldebouni]

Summary

Fixes #4675

Details and comments

when generating a controlled version of a gate, if a gate was unknown (not x, rx, ry, or rz), the gate gets unrolled to u1,u3,cx. The controlled version of u3 consists of multiple cRz and cRy which meant that simple circuits consisting of, but not exactly is, basic gates, gets transformed to long circuits.

This pr, makes the function add_control() unroll unknown gates to x,y,z in addition to u1,u3,cx. when generating the controlled version, it uses mcx gate to implement controlled x's, and a cz/cy to implement a controlled z's and y's (only when num of control is 1).

[CLAassistant]

All committers have signed the CLA.

[ajavadia]

As hinted in the original issue this seems more of a problem with how gates are treated with their names during circuit_to_gate conversion. If you control an X gate, there's no problem. But because X was a circuit first, it gets an autogenerated name when converted to gate, and then it is treated as a generic unitary which is inefficient.
So for the fix, rather than hardcoding Pauli gates, I think we need to treat the underlying issue. I'll let @ewinston and @Cryoris weigh in more.

[faisaldebouni]

You are correct. However if we just changed the name of the gate generated from circuit_to_gate, this will not cover the case when there are multiple x gates (and maybe other gates) in the original circuit. I think this is a more an issue of how the controlled gate is generated.

I believe there are three solutions (I could think of)
option 1: when controlling a generic gate, control each of it's definition gates recursively.
option 2: when deciding how to control the unrolled u3,u1 gates. if theta and lambda = pi, control using mcx rather than the more general mcrz,mcry,mcrz approach. Same for other special cases (y,z)
option 3: unroll using more gates (this pr)

I went with option 3 because it's the simplest. It covers more cases and makes the generated circuit shorter and plots nicer for other use cases. If a user doesn't want this behavior he can unroll the generated circuit or decompose it one more time, which is consistent with the flow of regular circuits.

---

Just to be clear, the problem is not that to_gate() is misnaming the gates. If you convert a circuit with an x gate in it for example, a gate is generated named circuit___ that contains in it's definition a gate properly named x. however when adding a control, add_control correctly considers circuit___ name.

[Cryoris]

I'm not sure this can be fixed by adapting the naming of circuit_to_gate since, like @faisaldebouni mentioned, we need to cover multiple single-qubit gates nested in one gate. But I might be a little biased because I'm not a fan of identifying the gates by their name instead of their type. Maybe I also didn't understand your comment correctly, @ajavadia 🤔

That being said; I think @faisaldebouni's option 1 would be the most correct approach. If we have a circuit with multiple circuits and gates nested inside we should recursively unroll the constituents until we find a block where a controlled version is available. Like

class Gate:  
     def control():
          return add_control(self)

def add_control(op):
   # cover special RZ and unitary cases
   return control(gate)

def control(gate):
    controlled_circuit = QuantumCircuit(n + 1)
    for op in gate:
        controlled_op = op.control()  # knows decomposition or will call add_control again and thus decompose
        controlled_circuit.append(controlled_op)  # modulo storing the right indices and applying to the right place
    return controlled_circuit.to_gate()

Option 3 is also feasible, but then we should unroll to all gates where we know the controlled version, i.e. to

x, y, z, h, rx, ry, rz, swap, cx, ccx, u1, u3

I'm not sure which option is is better for performance, though, maybe @kdk or @mtreinish also have an opinion on this. The advantage of option 1 is that we don't have to keep an up-to-date list of which gates we know how to control, however 3 is definitely simpler. I think both approaches are fine, with a preference for whichever is faster 🚀

[Cryoris]

Whichever solution we choose, @faisaldebouni could you add some tests checking that the controls are now computed correctly? 🙂

[Cryoris]

@faisaldebouni After discussing this we'd suggest to go with your option 3 for now, but please add unrolling for all the other gates too, as I mentioned in the comment above. After this release we can do another PR where we implement the recursive strategy.

[faisaldebouni]

Will do. Hope I can help when you decide to go with option 1.

For the tests. I assume that tests already exist to check if controlled gates are correct. Unless you mean to check that they are compact?
either way I'll make sure what ever we do here is covered

Also, how do we test that this will not cause performance issues?
Thanks for your guidance

[Cryoris]

For the tests you could just add something where you have a circuit with the single qubit gates that are unrolled to, control the circuit and check that the outcome is what you expect. Like

circuit = QuantumCircuit(1)
circuit.x(0)
circuit.rx(0.2, 0)
# etc

controlled = QuantumCircuit(2)
controlled.compose(circuit.control(), inplace=True)

expected = QuantumCircuit(2)
expected.cx(0, 1)
expected.crx(0.2, 0, 1)
# etc

self.assertEqual(controlled, expected)

[faisaldebouni]

I apologies for taking so long. I was struggling with a bug only to discover that is's not caused by this pr, rather it's present on current master as well

The bug occurs when controlling a circuit containing a single cnot. The resulting circuit maps the control and targets incorrectly. Running this code on master (without this pr) will result in an incorrect mapping of controls and target:

qc = QuantumCircuit(2)
qc.cx(1,0)

controlled = QuantumCircuit(3)
controlled.append(qc.to_gate().control(), [0,1,2])

controlled.decompose().draw()

It's worth mentioning that prior to decomposing, the circuit executes correctly with controls and target at the correct place. Moreover, this bug happens only with cx, and only when there is exactly a single cx in the original circuit. changing cx to cy for example or adding any gate to qc (even if another cx) causes the controls to be mapped correctly. I tried to debug this with no luck.

I've pushed the rest of this pr as the issue it addresses is different than this bug. However I don't believe that this pr should be merged before this bug is resolved. Thus I will leave this as draft

Final notes:
1- This PR doesn't unroll to rz since crz was causing a global phase which caused some tests to fail. also unrolling to u1 doesn't produce extra gates when controlling it, and cu1 is plotted like cz anyway.

2- This PR fails one of the tests it introduces due to mentioned bug.

[faisaldebouni]

I was able to find the cause of the bug. The decompose pass, when encountering an operation with a single operation in it's definition, it will simply swap the operation on the old node with the new operation, however this neglects to re-map node wires. Do I need to push a fix for this bug with this pr or should it be in a separate issue/pr?

simpler code to reproduce bug on master

qc = QuantumCircuit(3)
qc.mcx([0,2],1)

qc2 = QuantumCircuit(3)
qc2.append(qc.to_gate(),[0,1,2])
qc2.decompose().draw(output='mpl')

[Cryoris]

Thanks for the changes @faisaldebouni, I've got a few more comments but in general this looks good. Also the tests are nice 👍

Can we remove lines 111-123? Since we include x, rx, ... in the set of gates we unroll to, this should be covered by the part below. Meaning if the operation is just an X gate then unrolling will still leave it an X gate and we can use mcx, right?

[faisaldebouni]

regarding lines 111-123: I believe not. Because MCXGate.control calls this function (if control state is not none). In that case this function will be building the controlled version. With these lines this will be done simply and efficiently and without the need to unroll. unrolling an mcx gate will generate a huge circuit if num of controls were more than 2. I believe this is the only case that this part of the code gets executed since all other XGates (and it's controlled versions) define their own control functions anyway.

I share your distaste for conditioning code on gate name, but I'm afraid these lines have to stay 😄

[Cryoris]

I see, but then we can still remove the parts concerning the rotations from 111-123, right? And if we need this for MCX, shouldn't we also add it for MCU1?

[faisaldebouni]

I'm not sure about removing those either.

I want to be clear. These lines can be indeed removed but not because we're unrolling to Rx,Ry,Rz. When controlling an Rx gate (for example) if and when the gate gets unrolled, the unroller consider gate definition, which are in term of u1,u3,cx. Thus will be unrolled to the same set of gates before and after this pr. What unrolling to Rx,Ry,Rz does is making controlling a composite gate which contains these gates be done without effectively decomposing these gates.
simply put, controlling an Rx gate will, no matter what, give a controlled gate with base_gate = Rx and a number of control qubits. Thus the gate will be plotted the same no matter what. What removing these line will do is altering how this gate is defined internally (mcRx vs control each gate in unrolled definition)

However, I believe that this must be done in their respected class definitions, and not to call the generic add control function then add special cases there.

Side note: the special case (elif operation.name == 'rz':) is not reachable because of lines 58-66

[faisaldebouni]

should we close this as #4565 is merged?

[Cryoris]

No, #4565 doesn't cover all gates, it only unrolls to the ones where we know how to control for an arbitrary number of qubits. It doesn't cover cases such as Y -> CY or Swap -> CSwap.

The approach you added in this PR might end up recursing a lot, since Gate.control might call add_control again. I'm not sure if that adds some overhead or slows down things.

Another, simpler approach we could do (analogous to #4565) would be something like

if num_ctrl_qubits == 1:
    basis = # gates where we know singly-controlled version: x y z h rx ry etc.
else:
   basis = # gates where we know any number of controls: x u1 rx ry rz cx ccx

and then call the respective circuit methods. Would you like to try that approach (probably easier) or to benchmark your approach? 🙂

[faisaldebouni]

I don't think Gate.control will cause much recursion. Because these gates will get unrolled to u1,u3 on the second call. (for Rz, lines 56-64. will cause one extra call).

However, I agree with you that the logic should be: we unroll to gates that we know how to directly control.

What about extending these gates control functions so they don't require calling add_control?

[faisaldebouni]

There is an issue with how controlled phase is being handled.
this example would give the wrong circuit: (both before and after this pr)

qc = QuantumCircuit(1)
qc.sx(0)
Operator(qc.control(1)).equiv(Operator(SXGate().control(1)))

The global phase will not be correctly accounted for unless we unroll to sx and sx dagger (and any future gate that it's circuit is defined up to a global phase).

the last commit addresses this issue by making some changes to how the controlled phase is accounted for. This should make the generated controlled gate correct regardless of the set of gates that we unroll to.

If these changes are not appropriate, I will revert to the original code and unroll to sx and sx dagger instead. Though I don't believe that would be correct.

I also added a test that would have catched this bug.

[Cryoris]

Hey @faisaldebouni, it seems this bug has been taken care of as a by-product in 8f717d9. I've checked the cases you covered in this PR and they seem to be fixed (except CCX), so I'll go ahead and close this PR. Feel free to reopen if you think I missed something!

That being said, the way controlled gates are handled is still not ideal: Instead of hardcoding the gates which we know how to control, the control mechanism should directly check if the gate has a control implemented, see #6542.