`BlockCollapser` doesnt work with Clbits

[chriseclectic]

Environment

• Qiskit Terra version: 0.23.2
• Python version: 3.9
• Operating system: MacOS

What is happening?

I am trying to use BlockCollector and BlockCollapser with filter functions that include circuit ops with classical bits, which currently doesn't work.

How can we reproduce the issue?

Here is a simple example where first I tried using the BlockCollapser:

from qiskit import QuantumCircuit
from qiskit.circuit import Measure
from qiskit.converters import circuit_to_dag, dag_to_circuit, circuit_to_instruction
from qiskit.dagcircuit import BlockCollector, BlockCollapser

qc = QuantumCircuit(3)
qc.h(range(3))
qc.measure_all()

dag = circuit_to_dag(qc)

# Collect all measure instructions
blocks_meas = BlockCollector(dag).collect_all_matching_blocks(
    lambda node: isinstance(node.op, Measure),
    split_blocks=False,
    min_block_size=1
)

def _collapse_fn(block):
    op = circuit_to_instruction(block)
    op.name = "COLLAPSED"
    return op

# Collapse measure blocks to a single instruction
BlockCollapser(dag).collapse_to_operation(blocks_meas, _collapse_fn)

This raises raises CircuitError: "Bit 'Clbit(ClassicalRegister(3, 'meas'), 2)' is not in the circuit.

What should happen?

I would like to be able to use BlockCollapser (and implicitly DAGCircuit.replace_block_with_op) with blocks that also contain Clbits.

Any suggestions?

I took a stab at trying to modify the collapse functionto try different handling to include clbits in wire_pos_map and when DAGCircuit.replace_block_with_op

from qiskit.circuit import CircuitInstruction

def collapser(dag, blocks, collapse_fn):
    """Collapser"""

    global_index_map = {wire: idx for idx, wire in enumerate(dag.qubits)}
    global_index_map.update({wire: idx for idx, wire in enumerate(dag.clbits)})

    for block in blocks:
        # Find the set of qubits used in this block (which might be much smaller than
        # the set of all qubits).
        cur_qubits = set()
        cur_clbits = set()
        for node in block:
            cur_qubits.update(node.qargs)
            cur_clbits.update(node.cargs)

        # For reproducibility, order these qubits compatibly with the global order.
        sorted_qubits = sorted(cur_qubits, key=lambda x: global_index_map[x])
        sorted_clbits = sorted(cur_clbits, key=lambda x: global_index_map[x])

        # Construct a quantum circuit from the nodes in the block, remapping the qubits.
        wire_pos_map = dict((qb, ix) for ix, qb in enumerate(sorted_qubits))
        wire_pos_map.update(dict((qb, ix) for ix, qb in enumerate(sorted_clbits)))

        qc = QuantumCircuit(len(cur_qubits), len(cur_clbits))
        for node in block:
            remapped_qubits = [wire_pos_map[qarg] for qarg in node.qargs]
            remapped_clbits = [wire_pos_map[carg] for carg in node.cargs]
            qc.append(CircuitInstruction(node.op, remapped_qubits, remapped_clbits))

        # Collapse this quantum circuit into an operation.
        op = collapse_fn(qc)

        # Replace the block of nodes in the DAG by the constructed operation
        # (the function replace_block_with_op is implemented both in DAGCircuit and DAGDependency).
        dag.replace_block_with_op(block, op, wire_pos_map, cycle_check=False)

    return dag

and it seems to work if I draw the DAG:

But calling dag_to_circuit on the output returns a circuit without connection to clbits on the collapsed op:

        ┌───┐ ░ ┌────────────┐
   q_0: ┤ H ├─░─┤0           ├
        ├───┤ ░ │            │
   q_1: ┤ H ├─░─┤1 COLLAPSED ├
        ├───┤ ░ │            │
   q_2: ┤ H ├─░─┤2           ├
        └───┘ ░ └────────────┘
meas: 3/══════════════════════

which if i try and transpile or decompose gives error: DAGCircuitError: 'bit mapping invalid: expected 3, got 6'.

I dont know if this is an issue with my attempt to modify the function, with replace_block_with_op, or with dag_to_circuit.

[chriseclectic]

Full error trace for BlockCollapser(dag).collapse_to_operation(blocks_meas, _collapse_fn):

---------------------------------------------------------------------------
CircuitError                              Traceback (most recent call last)
Cell In [57], line 25
     22     return op
     24 # Collapse measure blocks to a single instruction
---> 25 BlockCollapser(dag).collapse_to_operation(blocks_meas, _collapse_fn)

File ~/mambaforge/envs/pec/lib/python3.9/site-packages/qiskit/dagcircuit/collect_blocks.py:282, in BlockCollapser.collapse_to_operation(self, blocks, collapse_fn)
    280 for node in block:
    281     remapped_qubits = [wire_pos_map[qarg] for qarg in node.qargs]
--> 282     qc.append(CircuitInstruction(node.op, remapped_qubits, node.cargs))
    284 # Collapse this quantum circuit into an operation.
    285 op = collapse_fn(qc)

File ~/mambaforge/envs/pec/lib/python3.9/site-packages/qiskit/circuit/quantumcircuit.py:1254, in QuantumCircuit.append(self, instruction, qargs, cargs)
   1251         operation = copy.deepcopy(operation)
   1253 expanded_qargs = [self.qbit_argument_conversion(qarg) for qarg in qargs or []]
-> 1254 expanded_cargs = [self.cbit_argument_conversion(carg) for carg in cargs or []]
   1256 if self._control_flow_scopes:
   1257     appender = self._control_flow_scopes[-1].append

File ~/mambaforge/envs/pec/lib/python3.9/site-packages/qiskit/circuit/quantumcircuit.py:1254, in <listcomp>(.0)
   1251         operation = copy.deepcopy(operation)
   1253 expanded_qargs = [self.qbit_argument_conversion(qarg) for qarg in qargs or []]
-> 1254 expanded_cargs = [self.cbit_argument_conversion(carg) for carg in cargs or []]
   1256 if self._control_flow_scopes:
   1257     appender = self._control_flow_scopes[-1].append

File ~/mambaforge/envs/pec/lib/python3.9/site-packages/qiskit/circuit/quantumcircuit.py:1148, in QuantumCircuit.cbit_argument_conversion(self, clbit_representation)
   1137 def cbit_argument_conversion(self, clbit_representation: ClbitSpecifier) -> List[Clbit]:
   1138     """
   1139     Converts several classical bit representations (such as indexes, range, etc.)
   1140     into a list of classical bits.
   (...)
   1146         List(tuple): Where each tuple is a classical bit.
   1147     """
-> 1148     return _bit_argument_conversion(
   1149         clbit_representation, self.clbits, self._clbit_indices, Clbit
   1150     )

File ~/mambaforge/envs/pec/lib/python3.9/site-packages/qiskit/circuit/quantumcircuit.py:4916, in _bit_argument_conversion(specifier, bit_sequence, bit_set, type_)
   4914     if specifier in bit_set:
   4915         return [specifier]
-> 4916     raise CircuitError(f"Bit '{specifier}' is not in the circuit.")
   4917 if isinstance(specifier, (int, np.integer)):
   4918     try:

CircuitError: "Bit 'Clbit(ClassicalRegister(3, 'meas'), 2)' is not in the circuit."

[chriseclectic]

Full error trace for

circ = dag_to_circuit(collapser(dag, blocks_meas, _collapse_fn))
circ.decompose()

---------------------------------------------------------------------------
DAGCircuitError                           Traceback (most recent call last)
Cell In [65], line 2
      1 circ = dag_to_circuit(collapser(dag, blocks_meas, _collapse_fn))
----> 2 circ.decompose()

File ~/mambaforge/envs/pec/lib/python3.9/site-packages/qiskit/circuit/quantumcircuit.py:1567, in QuantumCircuit.decompose(self, gates_to_decompose, reps)
   1565 pass_ = Decompose(gates_to_decompose)
   1566 for _ in range(reps):
-> 1567     dag = pass_.run(dag)
   1568 return dag_to_circuit(dag)

File ~/mambaforge/envs/pec/lib/python3.9/site-packages/qiskit/transpiler/passes/basis/decompose.py:65, in Decompose.run(self, dag)
     63         else:
     64             decomposition = circuit_to_dag(node.op.definition)
---> 65             dag.substitute_node_with_dag(node, decomposition)
     67 return dag

File ~/mambaforge/envs/pec/lib/python3.9/site-packages/qiskit/dagcircuit/dagcircuit.py:1203, in DAGCircuit.substitute_node_with_dag(self, node, input_dag, wires, propagate_condition)
   1197     node_wire_order += [
   1198         bit
   1199         for bit in self._bits_in_condition(getattr(node.op, "condition", None))
   1200         if bit not in node_cargs
   1201     ]
   1202 if len(wires) != len(node_wire_order):
-> 1203     raise DAGCircuitError(
   1204         f"bit mapping invalid: expected {len(node_wire_order)}, got {len(wires)}"
   1205     )
   1206 wire_map = dict(zip(wires, node_wire_order))
   1207 if len(wire_map) != len(node_wire_order):

DAGCircuitError: 'bit mapping invalid: expected 3, got 6'

[chriseclectic]

This line seems to be culprit for the custom function I wrote (which itself could be a fix for BlockCollapser): https://github.com/Qiskit/qiskit-terra/blob/main/qiskit/dagcircuit/dagcircuit.py#L1137-L1138

It seems cargs I being thrown away for everything that isn't a conditional op. If I change

if isinstance(nd, DAGOpNode) and getattr(nd.op, "condition", None):
    block_cargs |= set(nd.cargs)

to

block_cargs |= set(nd.cargs)

it seems to work for my use case, though I dont know if there are any other unintended concequences

[alexanderivrii]

Thanks @chriseclectic!

Indeed, the BlockCollapser code that I wrote does not treat clbits correctly, and your fix above is perfect.

And indeed, the dag_to_circuit code

if isinstance(nd, DAGOpNode) and getattr(nd.op, "condition", None):
    block_cargs |= set(nd.cargs)

seems to incorrectly drop clbits for non-conditional ops. It seems that I was the last to change these particular lines as well, however this behavior also existed in the code before my changes https://github.com/Qiskit/qiskit-terra/blob/ea0266769802a57de2ca823b1b996b668ec81178/qiskit/dagcircuit/dagcircuit.py#L1084-L1087, again your fix seems perfect, though let me double-check with @jakelishman or @mtreinish if there might be a reason to have this check in the code.

Out of curiosity, how are you planning to use the "COLLAPSED" blocks, I guess circ.decompose() would just result in the original circuit?

[chriseclectic]

@alexanderivrii While my above fix seemed to work for measurement blocks, I found it still didn't work for conditional blocks since conditional dag ops still report cargs=None, and only have the clbits hidden in node.op.condition. To get it to work for that case I ended up having to write a function like:

def _get_node_cargs(node):
    """Get cargs for possibly conditional node"""
    # Fix because DAG circuits hide conditional node cargs in
    # node.op.condition
    cargs = set(node.cargs)
    if node.op.condition:
        creg = node.op.condition[0]
        if isinstance(creg, Clbit):
            cargs.add(creg)
        else:
            cargs.update(creg)
    return cargs

to find all the cargs for a node that may or may not be conditional, and then use that in replace_block_with_ops like

for nd in node_block:
        block_qargs |= set(nd.qargs)
        block_cargs |= set(_get_node_cargs(nd))

There was some more complications with rewriting the collapse function because it had to correctly rewrite the conditioning in the circuit definition, which I could only mange to get to function if the conditioning was done in terms of individual clbits, not classical registers

[jakelishman]

Yeah, Chris your most recent comment looks logically correct to me (there's some superfluous set creations, but no big deal). I'm not sure what was going on with the current implementation - as you say, it just looks broken.

[alexanderivrii]

Thanks @chriseclectic! I have updated the PR with additional fixes based on your code. So we should be able to correctly handle measurement blocks (with explicit cargs), and conditional gates conditioned on classical bits (e.g., cx.c_if(clbit, 0) or cx.c_if(creg[1], 0). This still does not handle conditional gates conditioned on classical registers (e.g., cx.c_if(creg, 3)). I am expecting that handling cregs correctly will require bigger changes and should best be treated in a follow-up PR.