Fix inner gate placement for control flow blocks #10602
Conversation
This commit fixes a bug in the mpl visualization of control flow blocks. It was assuming the qubit order of the control flow block matched the qubit order in the outer circuit. This could result in the output visualization putting the gates from a control flow block on the incorrect qubits. This commit corrects this oversight by mapping the input qargs to the control flow op to the inner qubits in the block circuit. This results in placing the gates on the correct wires in the output visualization regardless of where the qubits in the circuit are. Fixes Qiskit#10601
|
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 5894601842
💛 - Coveralls |
|
also needs a reno |
There was a problem hiding this comment.
I think this might be a deeper problem. This circuit
qreg = QuantumRegister(2, "qr")
creg = ClassicalRegister(2, "cr")
qc = QuantumCircuit(qreg, creg)
qc.h([0, 1])
qc.h([0, 1])
qc.h([0, 1])
with qc.if_test((creg, 1)):
qc.x(0)
with qc.if_test((creg, 2)):
qc.z(0)
backend = FakeBelemV2()
backend.target.add_instruction(IfElseOp, name="if_else")
transpile(qc, backend, optimization_level=2, seed_transpiler=671_42).draw('mpl')
gives this error,
~/qiskit/qiskit-terra/qiskit/visualization/circuit/matplotlib.py in <dictcomp>(.0)
513 flow_wire_map = {
--> 514 inner: wire_map[outer]
515 for outer, inner in zip(node.qargs, circuit.qubits)
516 if inner not in wire_map
KeyError: Qubit(QuantumRegister(2, 'qr'), 0)
The wire map at this point is
{Qubit(QuantumRegister(5, 'q'), 0): 0, Qubit(QuantumRegister(5, 'q'), 1): 1, Qubit(QuantumRegister(5, 'q'), 2): 2, Qubit(QuantumRegister(5, 'q'), 3): 3, Qubit(QuantumRegister(5, 'q'), 4): 4, Clbit(ClassicalRegister(2, 'cr'), 0): 5, Clbit(ClassicalRegister(2, 'cr'), 1): 6}
|
Hmm, yeah it looks like the recursive mapping isn't working with nested control flow. I'll take a look and see if I can figure out what's going on. |
|
Not sure why the |
|
Yeah, I think part of it is just register naming changing now because there are more tests being executed and the implicit naming goes from something like q3 to q17. This is a general issue in the visual tests as they're sensitive to execution order since they don't set names on the registers. But I'll just update the references for this PR and in a separate one we can fix the test suite. |
|
Ah, yes. Implicit naming has caught me a few times. |
| if not flow_wire_map: | ||
| flow_wire_map = wire_map | ||
| else: | ||
| flow_wire_map.update(wire_map) |
There was a problem hiding this comment.
| if not flow_wire_map: | |
| flow_wire_map = wire_map | |
| else: | |
| flow_wire_map.update(wire_map) | |
| flow_wire_map.update(wire_map) |
Not really necessary to test.
There was a problem hiding this comment.
Tbh, I'm not sure why this update is necessary; when entering an inner scope, the only bits that I think should be accessible are the ones defined by the bit-binding relations in lines 511 to 523.
There was a problem hiding this comment.
In this circuit, the qc2 circuit can use either existing qr, cr or new qr2, cr2. If for example cr is used, the inner map will not contain this since it's already in the wire_map. This means without the combination above, it won't be available for the condition code.
qr = QuantumRegister(3, "q")
cr = ClassicalRegister(2, "cr")
qc = QuantumCircuit(qr, cr)
qc.h(0)
qr2 = QuantumRegister(3, "qr2")
cr2 = ClassicalRegister(2, "cr2")
qc2 = QuantumCircuit(qr, cr) # Can be (qr2, cr2)
qc2.x(1).c_if(cr[0], 1) # If cr2, change to cr2
with qc.if_test((cr[0], 1)):
qc.x(0)
qc.if_else((cr[1], 1), qc2, None, [0, 1, 2], [0, 1])
qc.y(0).c_if(cr[0], 0)
qc.draw("mpl", style={"showindex": True})
There was a problem hiding this comment.
If you make the changes suggested in your comments there, you'd have made an invalid circuit; it's not permitted to use a register condition that's not defined in the outermost scoped circuit. It's an unfortunate restriction and a gotcha, but it's required at the moment because we don't have any way of defining scoped storage locations for hardware, nor any way of defining a "binding" of registers from an outer circuit to an inner scope in the same way that we do for bits.
There was a problem hiding this comment.
Oh, OK. It doesn't raise an error, so I assumed it was acceptable and the inner circuit qubits/clbits would just get mapped to the outer ones.
There was a problem hiding this comment.
Yeah, there's no error checking of it; I guess we'd have to put a special case into QuantumCircuit.append to do that (it's not enough to put it in if_test / if_else because append is a valid entry point too, but _append's contract is that the user is explicitly responsible for all error checking). Maybe that would actually be a good idea - it shouldn't be too expensive, and it would help prevent common issues.
There was a problem hiding this comment.
Ah, I'd missed that we still have the if inner not in wire_map line in both dictionary comprehensions - my comment goes along with removing that restriction; the inner scopes should be exactly the bits in the zip(node.qargs, block.qubits) (and clbits) regardless of the inner one is already in the outer map or not.
Summary
This commit fixes a bug in the mpl visualization of control flow blocks. It was assuming the qubit order of the control flow block matched the qubit order in the outer circuit. This could result in the output visualization putting the gates from a control flow block on the incorrect qubits. This commit corrects this oversight by mapping the input qargs to the control flow op to the inner qubits in the block circuit. This results in placing the gates on the correct wires in the output visualization regardless of where the qubits in the circuit are.
Details and comments
Fixes #10601
Co-authored-by: Edwin Navarro enavarro@comcast.net