Fix potential non-determinism in DAGOpNode sort key (Qiskit#9569)

* Fix potential non-determinism in DAGOpNode sort key

This commit fixes potential source of non-determinism when topologically
sorting a DAGCircuit object. The lexicographical_topological_sort()
function we're using from rustworkx takes a sort key callback that
returns a string which is used for tie breaking in the topological sort.
The string used for op nodes was previously `str(qargs)`, but this is
potentially problematic for standalone Qubit objects. The repr for a
qubit without a register set will include the memory address of the bit
object. This could result in the topological sort differing between
executions of the same program which could lead to unexpected results.
This commit fixes this by changing the sort key to be a string of the
qubit indices in the dag instead of the qubit objects in qargs.

This commit will likely introduce a small performance regression because
we're adding overhead on every DAGOpNode creation. This regression can
be addressed when Qiskit#9389 is implemented because we can just pass the data
structure mapping bit objects to indices to the constructor instead of
having to build that mapping on each op node.

Additionally, some test cases were fixed as part of this commit, because
several DAGCircuit test cases were incorrectly calling
apply_operation_back() and setting clbits as qargs which will now fail
because of the index lookup on qubits.

* Update sort_key creation to use DAGCircuit.find_bit

* 0 pad integer indices and include cargs in sort key

This commit updates the sort key construction to do two things, first it
updates the integer indices used in the sort key to be 0 padded so that
the sort order is more intuitive. At the same time the sort key is not
including the cargs for an operation to ensure that's being factored
into the sort order.

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>

---------

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>

qiskit/dagcircuit/dagcircuit.py

@@ -600,7 +600,7 @@ def _add_op_node(self, op, qargs, cargs):
             int: The integer node index for the new op node on the DAG
         """
         # Add a new operation node to the graph
-        new_node = DAGOpNode(op=op, qargs=qargs, cargs=cargs)
+        new_node = DAGOpNode(op=op, qargs=qargs, cargs=cargs, dag=self)
         node_index = self._multi_graph.add_node(new_node)
         new_node._node_id = node_index
         self._increment_op(op)
@@ -1134,7 +1134,7 @@ def replace_block_with_op(self, node_block, op, wire_pos_map, cycle_check=True):
         block_qargs.sort(key=wire_pos_map.get)
         block_cargs = [bit for bit in block_cargs if bit in wire_pos_map]
         block_cargs.sort(key=wire_pos_map.get)
-        new_node = DAGOpNode(op, block_qargs, block_cargs)
+        new_node = DAGOpNode(op, block_qargs, block_cargs, dag=self)
 
         try:
             new_node._node_id = self._multi_graph.contract_nodes(
@@ -1345,7 +1345,7 @@ def edge_weight_map(wire):
                 m_op = old_node.op
             m_qargs = [wire_map[x] for x in old_node.qargs]
             m_cargs = [wire_map[x] for x in old_node.cargs]
-            new_node = DAGOpNode(m_op, qargs=m_qargs, cargs=m_cargs)
+            new_node = DAGOpNode(m_op, qargs=m_qargs, cargs=m_cargs, dag=self)
             new_node._node_id = new_node_index
             self._multi_graph[new_node_index] = new_node
             self._increment_op(new_node.op)

qiskit/dagcircuit/dagnode.py

@@ -13,6 +13,7 @@
 
 """Objects to represent the information at a node in the DAGCircuit."""
 
+import itertools
 import uuid
 from typing import Iterable
 
@@ -251,13 +252,18 @@ class DAGOpNode(DAGNode):
 
     __slots__ = ["op", "qargs", "cargs", "sort_key"]
 
-    def __init__(self, op, qargs: Iterable[Qubit] = (), cargs: Iterable[Clbit] = ()):
+    def __init__(self, op, qargs: Iterable[Qubit] = (), cargs: Iterable[Clbit] = (), dag=None):
         """Create an Instruction node"""
         super().__init__()
         self.op = op
         self.qargs = tuple(qargs)
         self.cargs = tuple(cargs)
-        self.sort_key = str(self.qargs)
+        if dag is not None:
+            self.sort_key = ",".join(
+                f"{dag.find_bit(q).index:04d}" for q in itertools.chain(self.qargs, self.cargs)
+            )
+        else:
+            self.sort_key = str(self.qargs)
 
     @property
     def name(self):

test/python/dagcircuit/test_dagcircuit.py

@@ -481,10 +481,10 @@ def test_apply_operation_back(self):
         x_gate.condition = self.condition
         self.dag.apply_operation_back(HGate(), [self.qubit0], [])
         self.dag.apply_operation_back(CXGate(), [self.qubit0, self.qubit1], [])
-        self.dag.apply_operation_back(Measure(), [self.qubit1, self.clbit1], [])
+        self.dag.apply_operation_back(Measure(), [self.qubit1], [self.clbit1])
         self.dag.apply_operation_back(x_gate, [self.qubit1], [])
-        self.dag.apply_operation_back(Measure(), [self.qubit0, self.clbit0], [])
-        self.dag.apply_operation_back(Measure(), [self.qubit1, self.clbit1], [])
+        self.dag.apply_operation_back(Measure(), [self.qubit0], [self.clbit0])
+        self.dag.apply_operation_back(Measure(), [self.qubit1], [self.clbit1])
         self.assertEqual(len(list(self.dag.nodes())), 16)
         self.assertEqual(len(list(self.dag.edges())), 17)
 
@@ -494,10 +494,10 @@ def test_edges(self):
         x_gate.condition = self.condition
         self.dag.apply_operation_back(HGate(), [self.qubit0], [])
         self.dag.apply_operation_back(CXGate(), [self.qubit0, self.qubit1], [])
-        self.dag.apply_operation_back(Measure(), [self.qubit1, self.clbit1], [])
+        self.dag.apply_operation_back(Measure(), [self.qubit1], [self.clbit1])
         self.dag.apply_operation_back(x_gate, [self.qubit1], [])
-        self.dag.apply_operation_back(Measure(), [self.qubit0, self.clbit0], [])
-        self.dag.apply_operation_back(Measure(), [self.qubit1, self.clbit1], [])
+        self.dag.apply_operation_back(Measure(), [self.qubit0], [self.clbit0])
+        self.dag.apply_operation_back(Measure(), [self.qubit1], [self.clbit1])
         out_edges = self.dag.edges(self.dag.output_map.values())
         self.assertEqual(list(out_edges), [])
         in_edges = self.dag.edges(self.dag.input_map.values())
@@ -786,7 +786,7 @@ def test_quantum_successors(self):
         #          ║
         #  c_1: 0 ═╩═══════════
 
-        self.dag.apply_operation_back(Measure(), [self.qubit1, self.clbit1], [])
+        self.dag.apply_operation_back(Measure(), [self.qubit1], [self.clbit1])
         self.dag.apply_operation_back(CXGate(), [self.qubit0, self.qubit1], [])
         self.dag.apply_operation_back(Reset(), [self.qubit0], [])
 
@@ -812,7 +812,7 @@ def test_quantum_successors(self):
 
     def test_is_successor(self):
         """The method dag.is_successor(A, B) checks if node B is a successor of A"""
-        self.dag.apply_operation_back(Measure(), [self.qubit1, self.clbit1], [])
+        self.dag.apply_operation_back(Measure(), [self.qubit1], [self.clbit1])
         self.dag.apply_operation_back(CXGate(), [self.qubit0, self.qubit1], [])
         self.dag.apply_operation_back(Reset(), [self.qubit0], [])
 
@@ -837,7 +837,7 @@ def test_quantum_predecessors(self):
 
         self.dag.apply_operation_back(Reset(), [self.qubit0], [])
         self.dag.apply_operation_back(CXGate(), [self.qubit0, self.qubit1], [])
-        self.dag.apply_operation_back(Measure(), [self.qubit1, self.clbit1], [])
+        self.dag.apply_operation_back(Measure(), [self.qubit1], [self.clbit1])
 
         predecessor_measure = self.dag.quantum_predecessors(self.dag.named_nodes("measure").pop())
         cnot_node = next(predecessor_measure)
@@ -929,7 +929,7 @@ def test_classical_successors(self):
     def test_is_predecessor(self):
         """The method dag.is_predecessor(A, B) checks if node B is a predecessor of A"""
 
-        self.dag.apply_operation_back(Measure(), [self.qubit1, self.clbit1], [])
+        self.dag.apply_operation_back(Measure(), [self.qubit1], [self.clbit1])
         self.dag.apply_operation_back(CXGate(), [self.qubit0, self.qubit1], [])
         self.dag.apply_operation_back(Reset(), [self.qubit0], [])
 
@@ -1312,10 +1312,10 @@ def test_layers_basic(self):
         dag.add_creg(creg)
         dag.apply_operation_back(HGate(), [qubit0], [])
         dag.apply_operation_back(CXGate(), [qubit0, qubit1], [])
-        dag.apply_operation_back(Measure(), [qubit1, clbit1], [])
+        dag.apply_operation_back(Measure(), [qubit1], [clbit1])
         dag.apply_operation_back(x_gate, [qubit1], [])
-        dag.apply_operation_back(Measure(), [qubit0, clbit0], [])
-        dag.apply_operation_back(Measure(), [qubit1, clbit1], [])
+        dag.apply_operation_back(Measure(), [qubit0], [clbit0])
+        dag.apply_operation_back(Measure(), [qubit1], [clbit1])
 
         layers = list(dag.layers())
         self.assertEqual(5, len(layers))

test/python/transpiler/test_sabre_layout.py

@@ -214,7 +214,7 @@ def test_layout_many_search_trials(self):
         self.assertIsInstance(res, QuantumCircuit)
         layout = res._layout.initial_layout
         self.assertEqual(
-            [layout[q] for q in qc.qubits], [22, 21, 4, 12, 1, 23, 16, 18, 19, 25, 14, 13, 10, 7]
+            [layout[q] for q in qc.qubits], [7, 19, 14, 18, 10, 6, 12, 16, 13, 20, 15, 21, 1, 17]
         )