Operator.from_circuit fails on transpiled circuits with user-defined registers

[jakelishman]

Environment

• Qiskit Terra version: main @ 293ab43
• Python version: 3.10
• Operating system: macOS

What is happening?

Operator.from_circuit throws a "cannot find bit" error when called on a transpiled circuit that was originally constructed out of:

• more than one register
• one register with a non-standard name (i.e. not q)
• loose qubits
• fewer qubits than the backend

How can we reproduce the issue?

All of these examples print "failed":

from qiskit import QuantumCircuit, QuantumRegister, transpile
from qiskit.circuit import Qubit
from qiskit.circuit.exceptions import CircuitError
from qiskit.providers.fake_provider import FakeBelem
from qiskit.quantum_info import Operator

backend5q = FakeBelem()

loose_bits = transpile(QuantumCircuit([Qubit() for _ in [None] * 5]), backend5q)
direct_register = transpile(QuantumCircuit(QuantumRegister(5)), backend5q)
two_registers = transpile(
    QuantumCircuit(QuantumRegister(3), QuantumRegister(2)), backend5q
)
not_big_enough = transpile(QuantumCircuit(3), backend5q)

for i, circuit in enumerate(
    (loose_bits, direct_register, two_registers, not_big_enough)
):
    try:
        Operator.from_circuit(circuit)
    except CircuitError:
        print(f"failed {i}")

The actual exception is along the lines of

KeyError                                  Traceback (most recent call last)
~/code/qiskit/terra/qiskit/circuit/quantumcircuit.py in find_bit(self, bit)
   1521             if isinstance(bit, Qubit):
-> 1522                 return self._qubit_indices[bit]
   1523             elif isinstance(bit, Clbit):

KeyError: <qiskit.circuit.quantumregister.Qubit object at 0x122f420c0>

The above exception was the direct cause of the following exception:

CircuitError                              Traceback (most recent call last)
<ipython-input-13-fe6a13093a55> in <module>
     17     (loose_bits, direct_register, two_registers, not_big_enough)
     18 ):
---> 19     Operator.from_circuit(circuit)
     20

~/code/qiskit/terra/qiskit/quantum_info/operators/operator.py in from_circuit(cls, circuit, ignore_set_layout, layout)
    236         # based on that layout
    237         if layout is not None:
--> 238             qargs = {
    239                 phys: circuit.find_bit(bit).index
    240                 for phys, bit in layout.get_physical_bits().items()

~/code/qiskit/terra/qiskit/quantum_info/operators/operator.py in <dictcomp>(.0)
    237         if layout is not None:
    238             qargs = {
--> 239                 phys: circuit.find_bit(bit).index
    240                 for phys, bit in layout.get_physical_bits().items()
    241             }

~/code/qiskit/terra/qiskit/circuit/quantumcircuit.py in find_bit(self, bit)
   1526                 raise CircuitError(f"Could not locate bit of unknown type: {type(bit)}")
   1527         except KeyError as err:
-> 1528             raise CircuitError(
   1529                 f"Could not locate provided bit: {bit}. Has it been added to the QuantumCircuit?"
   1530             ) from err

CircuitError: 'Could not locate provided bit: <qiskit.circuit.quantumregister.Qubit object at 0x122f420c0>. Has it been added to the QuantumCircuit?'

What should happen?

The bits we're looking up should be the ones that are actually on the circuit.

Any suggestions?

The current form only works (as best as I can tell) with circuits constructed as QuantumCircuit(n), where n is the number of bits in the backend used in the transpilation call. The fact that this works is a side-effect of how the legacy Qubit hash function works, and that the legacy behaviour of QuantumCircuit(n) is to create a register called "q", which is the same name as what the layout passes create by default.

The _layout field in a circuit from transpile has the same qubits as the input circuit in its "virtual bits" fields, but the returned circuit has a single register called "q" that fills everything. The lookup in Operator.find_circuit tries to use the virtual bits from the original circuit, but these don't exist any more.

I'm not actually sure if this is recoverable off the top of my head - I'm not certain Layout retains enough information to infer the order the virtual qubits of the previous circuit were in, especially if they are loose bits.

[mtreinish]

I tried to integrate a fix for this here: https://github.com/Qiskit/qiskit-terra/pull/8597/files#diff-f5222a986231cd8c6a8d2c71592e43aaf0ba310b123373fb47fe156d4dc34e72R244-R255 (if that PR doesn't make the release we can always extract it as a standalone bugfix). It is a tricky problem in general because as you say the Layout from transpile doesn't always provide us with sufficient information to do this. Since the output layout from transpile is the input circuit qubits to the output physical qubits (which are equivalent to the output virtual qubits index in the output circuit). That mapping doesn't give us sufficient information to know what order the input circuit's virtual qubits are in. The best we can do in the case where the virtual qubits aren't the same post transpilation (i.e. a flat register for all active qubits named "q") is to rely on insertion order but I'm not sure we're necessarily constructing the layout object in the same order as the qubits in the input circuit. We might need to rethink what we return from transpile for the ._layout attribute to handle this (or add an additional attribute for pre_layout qubits or something like that) to be able to do this robustly.

[jakelishman]

I'm not opposed to us adding a public QuantumCircuit.layout attribute (defaults to None), which will let us put whatever object we like there. I don't much like how _layout is private and pretty informally specified, but quite a few bits of Qiskit access it and do things based off it.

[mtreinish]

Well IIRC, when the _layout property was added we weren't sure that it was the interface we wanted to commit to which is why we made it private. I do think since we're trying to get better about this exposing this more having this be public makes sense now, but I'm thinking making it a tuple with like the Layout object and an optional list of circuit.qubits (defaulting to None) from the original pre-transpiled circuit (matching the order in the original circuit) might be the better interface. This would give us sufficient information to track the permutation correctly in all cases I think

[jakelishman]

Yeah, or perhaps a dataclass / some custom object that contains the original qubits and the layout? It's easier to extend an attribute-based object than a tuple if we need to add more later.

[mtreinish]

I pushed up a fix PR in #8802 and left the property as private for now (and used a tuple) to make sure we could include it in 0.22.0 at this point in the cycle. I'll open a new issue for 0.23.0 to make a public layout attribute and/or a public method to reverse the layout somehow and we can make a container class for it as part of that