Compose a circuit containing ControlFlowOps breaks drawer (and possibly other things)

[chriseclectic]

Environment

• Qiskit version: 1.0.1
• Python version: 3.11
• Operating system: MacOs

What is happening?

While writing unittests using teleportation circuits with various ways of doing conditionals I have run into errors with IfElseOp and SwitchOp where when they are composed into a new circuit, their internal blocks and target/conditions are not update to the new circuit Clbits and continue to reference the original circuit. This doesn't obviously fail until you try and do something with the circuit like draw it.

How can we reproduce the issue?

Here are 3 versions of a Bell teleport circuit:

from qiskit.circuit import QuantumCircuit, ClassicalRegister, QuantumRegister

qr = QuantumRegister(3, 'q')
cr = ClassicalRegister(1, 'c')
cr_aux = ClassicalRegister(2, 'aux')

# Use old style conditionals
qc1 = QuantumCircuit(qr, cr, cr_aux)
qc1.ry(par, 0)
qc1.h(1)
qc1.cx(1, 2)
qc1.cx(0, 1)
qc1.h(0)
qc1.measure(0, cr_aux[0])
qc1.z(2).c_if(cr_aux[0], 1)
qc1.measure(1, cr_aux[1])
qc1.x(2).c_if(cr_aux[1], 1)
qc1.measure(2, cr)

# Use IfElseOp
qc2 = QuantumCircuit(qr, cr, cr_aux)
qc2.h(1)
qc2.cx(1, 2)
qc2.cx(0, 1)
qc2.h(0)
qc2.measure(0, cr_aux[0])
with qc2.if_test((cr_aux[0], 1)): 
    qc2.z(2)
qc2.measure(1, cr_aux[1])
with qc2.if_test((cr_aux[1], 1)):
    qc2.x(2)
qc2.measure(2, cr)

# Use SwitchOp
qc3 = QuantumCircuit(qr, cr, cr_aux)
qc3.h(1)
qc3.cx(1, 2)
qc3.cx(0, 1)
qc3.h(0)
with qc3.switch(cr_aux) as case:
    with case(1):
        qc3.z(2)
    with case(2):
        qc3.x(2)
    with case(3):
        qc3.z(2)
        qc3.x(2)
qc3.measure(2, cr)

Now if for each case I do:

qc1_flat = QuantumCircuit(3, 3)
qc1_flat.compose(qc1, inplace=True) # etc
qc1_flat.draw()

The c_if case works as expected, but the if_test and switch cases both give errors coming from

KeyError: Clbit(ClassicalRegister(2, 'aux'), 1)

What should happen?

The above code example should work and return a valid circuit in all cases for composing circuits with control flow ops into larger circuits on a valid specification of qubits and clbits.

Any suggestions?

Digging into this it seems that there is internal data specific to these control flow ops that is not getting updated correctly when appending them to another circuit, and there is a lot of special handling in many locations for these instructions that uses this data.

Eg in the composed circuit, while the CircuitInstruction has updated qubits and clbits, the operation for these control flow ops constians the same block circuits as the original one, and these blocks reference the qubits and clbits in the original circuit they were constructed in, not the current circuit they were composed into.

Eg: for the switch case, in the composed circuit we have:

qc3_flat = QuantumCircuit(3, 3)
qc3_flat.compose(qc3, inplace=True)
for block in qc3_flat.data[-2].operation.blocks:
    print(block.clbits)

prints

[Clbit(ClassicalRegister(2, 'aux'), 1), Clbit(ClassicalRegister(2, 'aux'), 0)]
[Clbit(ClassicalRegister(2, 'aux'), 1), Clbit(ClassicalRegister(2, 'aux'), 0)]
[Clbit(ClassicalRegister(2, 'aux'), 1), Clbit(ClassicalRegister(2, 'aux'), 0)]

[jakelishman]

The most immediate problem here is (I believe) in the drawers, not in compose.

Eg in the composed circuit, while the CircuitInstruction has updated qubits and clbits, the operation for these control flow ops constians the same block circuits as the original one, and these blocks reference the qubits and clbits in the original circuit they were constructed in, not the current circuit they were composed into.

This part of the issue is not actually a bug. It's part of the data model (for better or worse) that the qubits within a ControlFlowOp block are not closure variables, they're just any arbitrary bits. Any consumer of the operations has to map dict(zip(block.qubits, instruction.qubits)) for each block in instruction.operation.blocks to map "inner" qubits to "outer" qubits. That might seem weird, but given that ControlFlowOp was stuffed into being a "regular" Instruction, it's basically required for the rest of the Instruction data model to work; when you have a compound Instruction, you're not required to have its qubits in its .definition field match the circuit it's going to be appended onto, but when you unroll a definition, you have to match the definition qubits with the CircuitInstruction.qubits etc. It's the same thing going on here, but there's way more things that need to recurse into control-flow operations in-place so they tend to turn up the complexity more.

The reason that the bits within a ControlFlowOp are ever the same as in the outer circuit is an implementation detail of the control-flow builder interface: for simplicity within itself, it just re-uses the Bit instances from the outer circuit that it resolves during appends.

It looks to me like the drawers are not getting the clbit binding quite right as they do their recursion. Just flicking very quickly through, they do do the correct bindings, but then I think they might be accidentally using the unbound objects in a couple of places to do look-ups.

[jakelishman]

I said I'd come here from #12084 and comment, not realising I already had. To expand more, since my previous comment didn't go into enough detail:

The if_test version is safe, and is producing a completely valid circuit, because its conditions are only on Clbit instances, which are safe. compose is correctly remapping them in the condition. It's fine that different Clbit instances are within the control-flow block, just like it's fine if an Instruction.definition circuit doesn't use the same Qubit instances that the outer circuit does. The bug seems to be entirely in the visualisers, which seem to have forgotten to indirect through the bit-remapping lookup when recursing into the control-flow operation in one particular place (it's like forgetting to look at CircuitInstruction.qubits when seeing what Instruction.definition is up to).

The switch version is not safe, and generally cannot be made safe within the limitations we have for ClassicalRegister, because it uses a register as typed data that has no direct analogue within the outer circuit. It's a bug that compose allows that to pass through without error, because it results in a broken QuantumCircuit; the switch instruction contains a reference to a ClassicalRegister that the circuit context does not know about. Qiskit could technically add an aliasing register to the outer circuit during the compose call - that's representable in our data model - but in practice, there's no hardware that supports that, nor is ever likely to, so it wouldn't improve the situation for you. In fact, it'd probably make things worse, since there'd be no indication that anything was wrong until it reached execution time in the hardware queue.

---

Qiskit 1.1 is adding the beginnings of the preferred way to rewrite the switch version for a way that's safe , which is local circuit classical variables (Vars). This is all super new, and it can't possibly be supported end-to-end through IBM hardware until Qiskit 1.1 is deployed on the backends (which also unfortunately means that there will be a delay and possibly a few bugs with it once Qiskit 1.1 releases on Thursday).

Give-or-take, though, this is one way you (hopefully!) will be able to rewrite that to work. Bear with the verbosity - this is new data model stuff, so the focus so far was in getting the representation in place, and less about getting the user interface as streamlined as it could be:

from qiskit.circuit import QuantumCircuit
from qiskit.circuit.classical import expr, types

qc3 = QuantumCircuit(3, 3)
# Explicit storage variable for the switch target.
target = qc3.add_var("target", expr.lift(0, types.Uint(2)))
qc3.h(1)
qc3.cx(1, 2)
qc3.cx(0, 1)
qc3.h(0)
qc3.measure([0, 1], [0, 1])
qc3.store(expr.index(target, 0), qc.clbits[0])
qc3.store(expr.index(target, 1), qc.clbits[1])
with qc3.switch(target) as case:
    with case(1):
        qc3.z(2)
    with case(2):
        qc3.x(2)
    with case(3):
        qc3.z(2)
        qc3.x(2)
qc3.measure(2, 2)

# ----

qc3_flat = QuantumCircuit(3, 3)
qc3_flat.compose(qc3, inplace=True)

(This example needs #12396 too because I found a bug in writing it!)

When using new-style Var nodes with compose, there will be two options. Either you:

• arrange that all Var nodes in each circuit to be composed are uniquely named, in which case compose with its default settings will add the Vars onto the base circuit.
• ensure that all the Var nodes you're going to want to use are already on the base circuit, then declare the relevant ones as "captures" on the smaller circuits, and use the inline_captures=True argument to compose to have it link those with the existing variables on the outer circuit.

There is potentially some scope for expanding the capabilities of compose in the future, but the aim for 1.1 was to have the base data model for typed classical variables working, so it can be expanded in the future. It requires you to be very explicit right now, but what we really don't want to do is end up in the ClassicalRegister situation where we didn't get the data model right in the beginning, and now there's so much implicit behaviour and additional semantics attached to them by other places in the stack that it becomes impossible for us to change them (plus, ClassicalRegister is way more limited in classical typing that what we really want anyway).