Add internal routine for canonicalisation of control-flow builders (#…

…8627)

* Add canonical form for control-flow builders

This adds an internal function for use in tests that puts circuits
constructed with the control-flow builders into a canonical form.  There
are several non-determinstic components to the control-flow builders,
since they internally track their resources with hashmaps, and at the
end pick an arbitrary order to sequence their arguments in.  The order
is not likely to be the same between a circuit constructed manually and
one constructed by a builder, even though they describe the same
dynamics.

For-loops with automatically generated loop variables (the
`with qc.for_loop(indices) as param: ...` form) will also vary between
circuits, as a unique parameter is constructed each time a new one is
requested to avoid collisions.  The canonicalisation routine uses a
shared iterable of parameters that always returns the exact instances in
the same order.  Replacing the loop variable with a new name has no
effect, since it is logically scoped to the loop only and the operation
is effectively lambda-calculus-like alpha conversion.

This canonicalisation routine is very slow and inefficient, so is only
intended for ease in writing tests.  Making the builders always output a
canonical order would add unnecessary overhead to them for most cases,
and the loop-parameter canonicalisation would potentially cause problems
if one control-flow built circuit was nested somehow within another
(such as by `QuantumCircuit.compose`), as the loop variables could
clash (and would violate the canonical form anyway).

* Fix non-deterministic DAGCircuit.__eq__

The bits-in-registers equality check previously constructed a sequence
of `(name, bit_indices)` tuples using the `dict.items()` iterator.  This
is unnecessarily insertion-order dependent for registers (in CPython,
non-deterministic in general).  Instead, we can compare two
dictionaries, which will correctly check key-by-key without needing the
creation order to match.

* Use canonicalisation instead of custom equivalence in builder tests

The builder tests previously used a custom circuit-equality checker,
which somewhat separated them from other circuit-equality tests.  Using
the control-flow aware canonicalisation routine first lets us rewrite
the circuit using simple rules that clearly don't change the semantics
of the circuit, and then use the regular equality checkers.

This swap over turned up a bug in the previous iteration of the tests
(in the "expected" results!), where the bits of two block bodies were
not ordered the same as the arguments to those operations.  This commit
also corrects those.

* Make initial recursive call clearer

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

qiskit/dagcircuit/dagcircuit.py

@@ -985,21 +985,19 @@ def __eq__(self, other):
         self_bit_indices = {bit: idx for idx, bit in enumerate(self.qubits + self.clbits)}
         other_bit_indices = {bit: idx for idx, bit in enumerate(other.qubits + other.clbits)}
 
-        self_qreg_indices = [
-            (regname, [self_bit_indices[bit] for bit in reg]) for regname, reg in self.qregs.items()
-        ]
-        self_creg_indices = [
-            (regname, [self_bit_indices[bit] for bit in reg]) for regname, reg in self.cregs.items()
-        ]
-
-        other_qreg_indices = [
-            (regname, [other_bit_indices[bit] for bit in reg])
-            for regname, reg in other.qregs.items()
-        ]
-        other_creg_indices = [
-            (regname, [other_bit_indices[bit] for bit in reg])
-            for regname, reg in other.cregs.items()
-        ]
+        self_qreg_indices = {
+            regname: [self_bit_indices[bit] for bit in reg] for regname, reg in self.qregs.items()
+        }
+        self_creg_indices = {
+            regname: [self_bit_indices[bit] for bit in reg] for regname, reg in self.cregs.items()
+        }
+
+        other_qreg_indices = {
+            regname: [other_bit_indices[bit] for bit in reg] for regname, reg in other.qregs.items()
+        }
+        other_creg_indices = {
+            regname: [other_bit_indices[bit] for bit in reg] for regname, reg in other.cregs.items()
+        }
         if self_qreg_indices != other_qreg_indices or self_creg_indices != other_creg_indices:
             return False
 

qiskit/test/_canonical.py

@@ -0,0 +1,125 @@
+# This code is part of Qiskit.
+#
+# (C) Copyright IBM 20022.
+#
+# This code is licensed under the Apache License, Version 2.0. You may
+# obtain a copy of this license in the LICENSE.txt file in the root directory
+# of this source tree or at http://www.apache.org/licenses/LICENSE-2.0.
+#
+# Any modifications or derivative works of this code must retain this
+# copyright notice, and modified files need to carry a notice indicating
+# that they have been altered from the originals.
+
+"""Utility methods for canonicalising various Qiskit objects, to help with testing."""
+
+import threading
+
+from qiskit.circuit import (
+    BreakLoopOp,
+    CircuitInstruction,
+    ContinueLoopOp,
+    ControlFlowOp,
+    ForLoopOp,
+    Parameter,
+    QuantumCircuit,
+)
+
+
+class _CanonicalParametersIterator:
+    """An object that, when iterated through, will produce the same sequence of parameters as every
+    other instance of this iterator."""
+
+    __parameters = []
+    __mutex = threading.Lock()
+
+    def __init__(self):
+        self._counter = 0
+
+    def __iter__(self):
+        return self
+
+    def __next__(self):
+        with self.__mutex:
+            if len(self.__parameters) >= self._counter:
+                param = Parameter(f"_canonicalization_loop_{self._counter}")
+                self.__parameters.append(param)
+            out = self.__parameters[self._counter]
+            self._counter += 1
+            return out
+
+
+def canonicalize_control_flow(circuit: QuantumCircuit) -> QuantumCircuit:
+    """Canonicalize all control-flow operations in a circuit.
+
+    This is not an efficient operation, and does not affect any properties of the circuit.  Its
+    intent is to normalise parts of circuits that have a non-deterministic construction.  These are
+    the ordering of bit arguments in control-flow blocks output by the builder interface, and
+    automatically generated ``for``-loop variables.
+
+    The canonical form sorts the bits in the arguments of these operations so that they always
+    appear in the order they were originally added to the outer-most circuit.  For-loop variables
+    are re-bound into new, cached auto-generated ones."""
+    params = iter(_CanonicalParametersIterator())
+    base_bit_order = {bit: i for i, bit in enumerate(circuit.qubits)}
+    base_bit_order.update((bit, i) for i, bit in enumerate(circuit.clbits))
+
+    def worker(circuit, bit_map=None):
+        if bit_map is None:
+            bit_map = {bit: bit for bits in (circuit.qubits, circuit.clbits) for bit in bits}
+
+        def bit_key(bit):
+            return base_bit_order[bit_map[bit]]
+
+        # This isn't quite QuantumCircuit.copy_empty_like because of the bit reordering.
+        out = QuantumCircuit(
+            sorted(circuit.qubits, key=bit_key),
+            sorted(circuit.clbits, key=bit_key),
+            *circuit.qregs,
+            *circuit.cregs,
+            name=circuit.name,
+            global_phase=circuit.global_phase,
+            metadata=circuit.metadata,
+        )
+        for instruction in circuit.data:
+            new_instruction = instruction
+            # Control-flow operations associated bits in the instruction arguments with bits in the
+            # circuit blocks just by sequencing.  All blocks must have the same width.
+            if isinstance(new_instruction.operation, ControlFlowOp):
+                op = new_instruction.operation
+                first_block = op.blocks[0]
+                inner_bit_map = dict(
+                    zip(first_block.qubits, (bit_map[bit] for bit in new_instruction.qubits))
+                )
+                inner_bit_map.update(
+                    zip(first_block.clbits, (bit_map[bit] for bit in new_instruction.clbits))
+                )
+                new_instruction = CircuitInstruction(
+                    operation=op.replace_blocks(
+                        [worker(block, inner_bit_map) for block in op.blocks]
+                    ),
+                    qubits=sorted(new_instruction.qubits, key=bit_key),
+                    clbits=sorted(new_instruction.clbits, key=bit_key),
+                )
+            elif isinstance(new_instruction.operation, (BreakLoopOp, ContinueLoopOp)):
+                new_instruction = new_instruction.replace(
+                    qubits=sorted(new_instruction.qubits, key=bit_key),
+                    clbits=sorted(new_instruction.clbits, key=bit_key),
+                )
+            # For for loops specifically, the control-flow builders generate a loop parameter if one
+            # is needed but not explicitly supplied.  We want the parameters to compare equal, so we
+            # replace them with those from a shared list.
+            if isinstance(new_instruction.operation, ForLoopOp):
+                old_op = new_instruction.operation
+                indexset, loop_param, body = old_op.params
+                if loop_param is not None:
+                    new_loop_param = next(params)
+                    new_op = ForLoopOp(
+                        indexset,
+                        new_loop_param,
+                        body.assign_parameters({loop_param: new_loop_param}),
+                    )
+                    new_instruction = new_instruction.replace(operation=new_op)
+            out._append(new_instruction)
+        return out
+
+    return worker(circuit)

releasenotes/notes/fix-nondeterministic-dagcircuit-eq-7caa9041093c6e4c.yaml

@@ -0,0 +1,8 @@
+---
+fixes:
+  - |
+    Comparing :class:`.QuantumCircuit` and :class:`.DAGCircuit`\ s for equality
+    was previously non-deterministic if the circuits contained more than one
+    register of the same type (*e.g.* two or more :class:`.QuantumRegister`\ s),
+    sometimes returning ``False`` even if the registers were identical.  It will
+    now correctly compare circuits with multiple registers.