Fix QuantumCircuit.compose on unusual types and registers (#9206)

* Fix `QuantumCircuit.compose` on unusual types and registers

`QuantumCircuit.compose` previously had its own handling for converting
bit specifiers into bit instances, meaning its functionality was often
surprisingly less permissive than `append`, or just incorrect.  This
swaps the method to use the standard conversion utilities.

The logic for handling mapping conditional registers was previously the
old logic in `DAGCircuit`, which was called without respect to
re-ordering of the clbits during the composition.  The existing test case
surrounding this that is modified by this commit made an incorrect
assertion; in general, the condition was not the same after the
composition.  In this particular test case it was ok because both bits
were being tested for the same value, but had the condition value been
0b01 or 0b10 it would have been incorrect.

This commit updates the register-mapping logic to respect the reordering
of clbits, and to create a new aliasing register to ensure the condition
is represented exactly, if this is required.  This fixes a category of
bug where too-small registers could incorrectly be mapped to larger
registers.  This was not a valid transformation, because it created
assertions about bits that were previously not involved in the
condition's comparison.

* Remove now-outdated DAG compose test

This test was actually attempting to catch invalid state of
`QuantumCircuit.compose`, but this series of commits changes the
underlying function so that this test no longer needs to raise; there is
valid alternative behaviour.

* Fix lint

* Add comment explaining list copy

* Update test/python/circuit/test_compose.py

Co-authored-by: Julien Gacon <gaconju@gmail.com>

Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

qiskit/circuit/quantumcircuit.py

@@ -771,8 +771,8 @@ def control(
     def compose(
         self,
         other: Union["QuantumCircuit", Instruction],
-        qubits: Optional[Sequence[Union[Qubit, int]]] = None,
-        clbits: Optional[Sequence[Union[Clbit, int]]] = None,
+        qubits: Optional[Union[QubitSpecifier, Sequence[QubitSpecifier]]] = None,
+        clbits: Optional[Union[ClbitSpecifier, Sequence[ClbitSpecifier]]] = None,
         front: bool = False,
         inplace: bool = False,
         wrap: bool = False,
@@ -842,98 +842,101 @@ def compose(
                 "Cannot emit a new composed circuit while a control-flow context is active."
             )
 
-        if inplace:
-            dest = self
-        else:
-            dest = self.copy()
+        dest = self if inplace else self.copy()
 
-        # If self does not have any cregs but other does then we allow
-        # that and add the registers to the output dest
+        # As a special case, allow composing some clbits onto no clbits - normally the destination
+        # has to be strictly larger. This allows composing final measurements onto unitary circuits.
         if isinstance(other, QuantumCircuit):
             if not self.clbits and other.clbits:
                 dest.add_bits(other.clbits)
                 for reg in other.cregs:
                     dest.add_register(reg)
 
-        if wrap:
-            try:
-                other = other.to_gate()
-            except QiskitError:
-                other = other.to_instruction()
+        if wrap and isinstance(other, QuantumCircuit):
+            other = (
+                other.to_gate()
+                if all(isinstance(ins.operation, Gate) for ins in other.data)
+                else other.to_instruction()
+            )
 
         if not isinstance(other, QuantumCircuit):
             if qubits is None:
-                qubits = list(range(other.num_qubits))
-
+                qubits = self.qubits[: other.num_qubits]
             if clbits is None:
-                clbits = list(range(other.num_clbits))
-
+                clbits = self.clbits[: other.num_clbits]
             if front:
-                dest.data.insert(0, CircuitInstruction(other, qubits, clbits))
+                # Need to keep a reference to the data for use after we've emptied it.
+                old_data = list(dest.data)
+                dest.clear()
+                dest.append(other, qubits, clbits)
+                for instruction in old_data:
+                    dest._append(instruction)
             else:
                 dest.append(other, qargs=qubits, cargs=clbits)
-
             if inplace:
                 return None
             return dest
 
-        instrs = other.data
-
         if other.num_qubits > dest.num_qubits or other.num_clbits > dest.num_clbits:
             raise CircuitError(
                 "Trying to compose with another QuantumCircuit which has more 'in' edges."
             )
 
         # number of qubits and clbits must match number in circuit or None
-        identity_qubit_map = dict(zip(other.qubits, dest.qubits))
-        identity_clbit_map = dict(zip(other.clbits, dest.clbits))
+        edge_map = {}
         if qubits is None:
-            qubit_map = identity_qubit_map
-        elif len(qubits) != len(other.qubits):
-            raise CircuitError(
-                f"Number of items in qubits parameter ({len(qubits)}) does not"
-                f" match number of qubits in the circuit ({len(other.qubits)})."
-            )
+            edge_map.update(zip(other.qubits, dest.qubits))
         else:
-            qubit_map = {
-                other.qubits[i]: (self.qubits[q] if isinstance(q, int) else q)
-                for i, q in enumerate(qubits)
-            }
+            mapped_qubits = dest.qbit_argument_conversion(qubits)
+            if len(mapped_qubits) != len(other.qubits):
+                raise CircuitError(
+                    f"Number of items in qubits parameter ({len(mapped_qubits)}) does not"
+                    f" match number of qubits in the circuit ({len(other.qubits)})."
+                )
+            edge_map.update(zip(other.qubits, mapped_qubits))
+
         if clbits is None:
-            clbit_map = identity_clbit_map
-        elif len(clbits) != len(other.clbits):
-            raise CircuitError(
-                f"Number of items in clbits parameter ({len(clbits)}) does not"
-                f" match number of clbits in the circuit ({len(other.clbits)})."
-            )
+            edge_map.update(zip(other.clbits, dest.clbits))
         else:
-            clbit_map = {
-                other.clbits[i]: (self.clbits[c] if isinstance(c, int) else c)
-                for i, c in enumerate(clbits)
-            }
-
-        edge_map = {**qubit_map, **clbit_map} or {**identity_qubit_map, **identity_clbit_map}
+            mapped_clbits = dest.cbit_argument_conversion(clbits)
+            if len(mapped_clbits) != len(other.clbits):
+                raise CircuitError(
+                    f"Number of items in clbits parameter ({len(mapped_clbits)}) does not"
+                    f" match number of clbits in the circuit ({len(other.clbits)})."
+                )
+            edge_map.update(zip(other.clbits, dest.cbit_argument_conversion(clbits)))
 
         mapped_instrs = []
-        for instr in instrs:
+        condition_register_map = {}
+        for instr in other.data:
             n_qargs = [edge_map[qarg] for qarg in instr.qubits]
             n_cargs = [edge_map[carg] for carg in instr.clbits]
-            n_instr = instr.operation.copy()
-
-            if getattr(instr.operation, "condition", None) is not None:
-                from qiskit.dagcircuit import DAGCircuit  # pylint: disable=cyclic-import
+            n_op = instr.operation.copy()
 
-                n_instr.condition = DAGCircuit._map_condition(
-                    edge_map, instr.operation.condition, self.cregs
-                )
+            # Map their registers over to ours, adding an extra one if there's no exact match.
+            if getattr(n_op, "condition", None) is not None:
+                target, value = n_op.condition
+                if isinstance(target, Clbit):
+                    n_op.condition = (edge_map[target], value)
+                else:
+                    if target.name not in condition_register_map:
+                        mapped_bits = [edge_map[bit] for bit in target]
+                        for our_creg in dest.cregs:
+                            if mapped_bits == list(our_creg):
+                                new_target = our_creg
+                                break
+                        else:
+                            new_target = ClassicalRegister(bits=[edge_map[bit] for bit in target])
+                            dest.add_register(new_target)
+                        condition_register_map[target.name] = new_target
+                    n_op.condition = (condition_register_map[target.name], value)
 
-            mapped_instrs.append(CircuitInstruction(n_instr, n_qargs, n_cargs))
+            mapped_instrs.append(CircuitInstruction(n_op, n_qargs, n_cargs))
 
         if front:
             # adjust new instrs before original ones and update all parameters
             mapped_instrs += dest.data
-            dest.data.clear()
-            dest._parameter_table.clear()
+            dest.clear()
         append = dest._control_flow_scopes[-1].append if dest._control_flow_scopes else dest._append
         for instr in mapped_instrs:
             append(instr)

releasenotes/notes/fix-compose-35d2fdbe5b052bca.yaml

@@ -0,0 +1,16 @@
+---
+fixes:
+  - |
+    The method :meth:`.QuantumCircuit.compose` now accepts the same set of qubit and clbit
+    specifiers as other :class:`.QuantumCircuit` methods, such as :meth:`~.QuantumCircuit.append`.
+    This means, for example, that Numpy integers will work.  Fixed `#8691
+    <https://github.com/Qiskit/qiskit-terra/issues/8691>`__.
+  - |
+    Fixed :meth:`.QuantumCircuit.compose` incorrectly mapping registers in conditions on the given
+    circuit to complete registers on the base.  Previously, the mapping was very imprecise; the bits
+    used within each condition were not subject to the mapping, and instead an inaccurate attempt was
+    made to find a corresponding register.  This could also result in a condition on a smaller register
+    being expanded to be on a larger register, which is not a valid transformation.  Now, a
+    condition on a single bit or a register will be composed to be on precisely the bits as defined
+    by the ``clbits`` argument.  A new aliasing register will be added to the base circuit to
+    facilitate this, if necessary.  Fixed `#6583 <https://github.com/Qiskit/qiskit-terra/issues/8691>__`.

test/python/circuit/test_compose.py

@@ -16,6 +16,8 @@
 
 import unittest
 
+import numpy as np
+
 from qiskit import transpile
 from qiskit.pulse import Schedule
 from qiskit.circuit import (
@@ -107,6 +109,30 @@ def test_compose_inorder(self):
         circuit_composed = self.circuit_left.compose(circuit_right, inplace=False)
         self.assertEqual(circuit_composed, circuit_expected)
 
+    def test_compose_inorder_unusual_types(self):
+        """Test that composition works in order, using Numpy integer types as well as regular
+        integer types.  In general, it should be permissible to use any of the same `QubitSpecifier`
+        types (or similar for `Clbit`) that `QuantumCircuit.append` uses."""
+        qreg = QuantumRegister(5, "rqr")
+        creg = ClassicalRegister(2, "rcr")
+        circuit_right = QuantumCircuit(qreg, creg)
+        circuit_right.cx(qreg[0], qreg[3])
+        circuit_right.x(qreg[1])
+        circuit_right.y(qreg[2])
+        circuit_right.z(qreg[4])
+        circuit_right.measure([0, 1], [0, 1])
+
+        circuit_expected = self.circuit_left.copy()
+        circuit_expected.cx(self.left_qubit0, self.left_qubit3)
+        circuit_expected.x(self.left_qubit1)
+        circuit_expected.y(self.left_qubit2)
+        circuit_expected.z(self.left_qubit4)
+        circuit_expected.measure(self.left_qubit0, self.left_clbit0)
+        circuit_expected.measure(self.left_qubit1, self.left_clbit1)
+
+        circuit_composed = self.circuit_left.compose(circuit_right, np.arange(5), slice(0, 2))
+        self.assertEqual(circuit_composed, circuit_expected)
+
     def test_compose_inorder_inplace(self):
         """Composing two circuits of the same width, default order, inplace.
 
@@ -397,9 +423,9 @@ def test_compose_conditional(self):
         lqr_2_1: ──┤ X ├────┤ X ├────┼───┤M├─╫─
                    └───┘    └─┬─┘    │   └╥┘ ║
                            ┌──┴──┐┌──┴──┐ ║  ║
-        lcr_0: ════════════╡     ╞╡     ╞═╩══╬═
-                           │ = 3 ││ = 3 │    ║
-        lcr_1: ════════════╡     ╞╡     ╞════╩═
+        lcr_0: ════════════╡     ╞╡     ╞═╬══╩═
+                           │ = 3 ││ = 3 │ ║
+        lcr_1: ════════════╡     ╞╡     ╞═╩════
                            └─────┘└─────┘
         """
         qreg = QuantumRegister(2, "rqr")
@@ -411,16 +437,46 @@ def test_compose_conditional(self):
         circuit_right.measure(qreg, creg)
 
         # permuted subset of qubits and clbits
-        circuit_composed = self.circuit_left.compose(circuit_right, qubits=[1, 4], clbits=[1, 0])
+        circuit_composed = self.circuit_left.compose(circuit_right, qubits=[1, 4], clbits=[0, 1])
 
         circuit_expected = self.circuit_left.copy()
         circuit_expected.x(self.left_qubit4).c_if(*self.condition)
         circuit_expected.h(self.left_qubit1).c_if(*self.condition)
-        circuit_expected.measure(self.left_qubit4, self.left_clbit0)
-        circuit_expected.measure(self.left_qubit1, self.left_clbit1)
+        circuit_expected.measure(self.left_qubit1, self.left_clbit0)
+        circuit_expected.measure(self.left_qubit4, self.left_clbit1)
 
         self.assertEqual(circuit_composed, circuit_expected)
 
+    def test_compose_conditional_no_match(self):
+        """Test that compose correctly maps registers in conditions to the new circuit, even when
+        there are no matching registers in the destination circuit.
+
+        Regression test of gh-6583 and gh-6584."""
+        right = QuantumCircuit(QuantumRegister(3), ClassicalRegister(1), ClassicalRegister(1))
+        right.h(1)
+        right.cx(1, 2)
+        right.cx(0, 1)
+        right.h(0)
+        right.measure([0, 1], [0, 1])
+        right.z(2).c_if(right.cregs[0], 1)
+        right.x(2).c_if(right.cregs[1], 1)
+        test = QuantumCircuit(3, 3).compose(right, range(3), range(2))
+        z = next(ins.operation for ins in test.data[::-1] if ins.operation.name == "z")
+        x = next(ins.operation for ins in test.data[::-1] if ins.operation.name == "x")
+        # The registers should have been mapped, including the bits inside them.  Unlike the
+        # previous test, there are no matching registers in the destination circuit, so the
+        # composition needs to add new registers (bit groupings) over the existing mapped bits.
+        self.assertIsNot(z.condition, None)
+        self.assertIsInstance(z.condition[0], ClassicalRegister)
+        self.assertEqual(len(z.condition[0]), len(right.cregs[0]))
+        self.assertIs(z.condition[0][0], test.clbits[0])
+        self.assertEqual(z.condition[1], 1)
+        self.assertIsNot(x.condition, None)
+        self.assertIsInstance(x.condition[0], ClassicalRegister)
+        self.assertEqual(len(x.condition[0]), len(right.cregs[1]))
+        self.assertEqual(z.condition[1], 1)
+        self.assertIs(x.condition[0][0], test.clbits[1])
+
     def test_compose_gate(self):
         """Composing with a gate.
 

test/python/dagcircuit/test_compose.py

@@ -16,7 +16,6 @@
 
 from qiskit.circuit import QuantumRegister, ClassicalRegister, QuantumCircuit
 from qiskit.converters import circuit_to_dag, dag_to_circuit
-from qiskit.dagcircuit.exceptions import DAGCircuitError
 from qiskit.test import QiskitTestCase
 from qiskit.pulse import Schedule
 from qiskit.circuit.gate import Gate
@@ -424,31 +423,6 @@ def test_compose_condition_multiple_classical(self):
 
         self.assertEqual(dag_composed, dag_expected)
 
-    def test_compose_raises_if_splitting_condition_creg(self):
-        """Verify compose raises if a condition is mapped to more than one creg.
-
-                             ┌───┐
-        q_0:           q_0: ─┤ H ├─
-                             └─┬─┘
-        c0: 1/  +           ┌──┴──┐   = DAGCircuitError
-                       c: 2/╡ = 2 ╞
-        c1: 1/              └─────┘
-        """
-
-        qreg = QuantumRegister(1)
-        creg1 = ClassicalRegister(1)
-        creg2 = ClassicalRegister(1)
-
-        circuit_left = QuantumCircuit(qreg, creg1, creg2)
-
-        wide_creg = ClassicalRegister(2)
-
-        circuit_right = QuantumCircuit(qreg, wide_creg)
-        circuit_right.h(0).c_if(wide_creg, 2)
-
-        with self.assertRaisesRegex(DAGCircuitError, "more than one creg"):
-            circuit_left.compose(circuit_right)
-
     def test_compose_calibrations(self):
         """Test that compose carries over the calibrations."""
         dag_cal = QuantumCircuit(1)