Node equality should be based on the internal id

qiskit/dagcircuit/dagcircuit.py

@@ -754,21 +754,17 @@ def _full_pred_succ_maps(self, pred_map, succ_map, input_circuit,
         return full_pred_map, full_succ_map
 
     def __eq__(self, other):
+        # TODO this works but is a horrible way to do this
+        slf = copy.deepcopy(self.multi_graph)
+        oth = copy.deepcopy(other.multi_graph)
 
-        if nx.is_isomorphic(self.multi_graph, other.multi_graph):
-            # TODO this works but is a horrible way to do this
-            slf = copy.deepcopy(self.multi_graph)
-            oth = copy.deepcopy(other.multi_graph)
+        for node in slf.nodes:
+            slf.nodes[node]['node'] = node
+        for node in oth.nodes:
+            oth.nodes[node]['node'] = node
 
-            for node in slf.nodes:
-                slf.nodes[node]["node"] = node
-            for node in oth.nodes:
-                oth.nodes[node]["node"] = node
-
-            return nx.is_isomorphic(slf, oth, node_match=lambda x, y:
-                                    x['node'] == y['node'])
-
-        return False
+        return nx.is_isomorphic(slf, oth,
+                                node_match=lambda x, y: DAGNode.semantic_eq(x['node'], y['node']))
 
     def nodes_in_topological_order(self):
         """

qiskit/dagcircuit/dagnode.py

@@ -19,6 +19,7 @@ class DAGNode:
     be supplied to functions that take a node.
 
     """
+
     def __init__(self, data_dict, nid=-1):
         """ Create a node """
         self._node_id = nid
@@ -83,31 +84,6 @@ def wire(self):
             raise QiskitError('The node %s is not an input/output node' % str(self))
         return self.data_dict.get('wire')
 
-    def __eq__(self, other):
-
-        # if other is None
-        if not other:
-            return False
-
-        if isinstance(other, int):
-            return other == self._node_id
-
-        # For barriers, qarg order is not significant so compare as sets
-        if 'barrier' == self.name == other.name:
-            node1_qargs = set(self.qargs)
-            node2_qargs = set(other.qargs)
-
-            if node1_qargs != node2_qargs:
-                return False
-
-            # qargs must be equal, so remove them from the dict then compare
-            copy_self = {k: v for (k, v) in self.data_dict.items() if k != 'qargs'}
-            copy_other = {k: v for (k, v) in other.data_dict.items() if k != 'qargs'}
-
-            return copy_self == copy_other
-
-        return self.data_dict == other.data_dict
-
     def __lt__(self, other):
         return self._node_id < other._node_id
 
@@ -128,3 +104,22 @@ def __str__(self):
     def pop(self, val):
         """Remove the provided value from the dictionary"""
         del self.data_dict[val]
+
+    @staticmethod
+    def semantic_eq(node1, node2):
+        """
+        Check if DAG nodes are considered equivalent, e.g. as a node_match for nx.is_isomorphic.
+
+        Args:
+            node1 (DAGNode): A node to compare.
+            node2 (DAGNode): The other node to compare.
+
+        Return:
+            Bool: If node1 == node2
+        """
+
+        # For barriers, qarg order is not significant so compare as sets
+        if 'barrier' == node1.name == node2.name:
+            return set(node1.qargs) == set(node2.qargs)
+
+        return node1.data_dict == node2.data_dict

test/python/test_dagcircuit.py

@@ -247,18 +247,18 @@ def test_nodes_in_topological_order(self):
 
     def test_dag_ops_on_wire(self):
         """ Test that listing the gates on a qubit/classical bit gets the correct gates"""
-
         self.dag.apply_operation_back(CnotGate(self.qubit0, self.qubit1),
                                       [self.qubit0, self.qubit1])
         self.dag.apply_operation_back(HGate(self.qubit0), [self.qubit0])
 
         qbit = self.dag.qubits()[0]
-        self.assertEqual([1, 11, 12, 2], list(self.dag.nodes_on_wire(qbit)))
-        self.assertEqual([11, 12], list(self.dag.nodes_on_wire(qbit, only_ops=True)))
+        self.assertEqual([1, 11, 12, 2], [i._node_id for i in self.dag.nodes_on_wire(qbit)])
+        self.assertEqual([11, 12],
+                         [i._node_id for i in self.dag.nodes_on_wire(qbit, only_ops=True)])
 
         cbit = self.dag.clbits()[0]
-        self.assertEqual([7, 8], list(self.dag.nodes_on_wire(cbit)))
-        self.assertEqual([], list(self.dag.nodes_on_wire(cbit, only_ops=True)))
+        self.assertEqual([7, 8], [i._node_id for i in self.dag.nodes_on_wire(cbit)])
+        self.assertEqual([], [i._node_id for i in self.dag.nodes_on_wire(cbit, only_ops=True)])
 
         (reg, _) = qbit
         with self.assertRaises(DAGCircuitError):
@@ -351,6 +351,25 @@ def test_circuit_factors(self):
         self.assertEqual(self.dag.num_tensor_factors(), 2)
 
 
+class TestCircuitSpecialCases(QiskitTestCase):
+    """DAGCircuit test for special cases, usually for regression."""
+
+    def test_circuit_depth_with_repetition(self):
+        """ When cx repeat, they are not "the same".
+        See https://github.com/Qiskit/qiskit-terra/issues/1994
+        """
+        qr1 = QuantumRegister(2)
+        qr2 = QuantumRegister(2)
+        circ = QuantumCircuit(qr1, qr2)
+        circ.h(qr1[0])
+        circ.cx(qr1[1], qr2[1])
+        circ.cx(qr1[1], qr2[1])
+        circ.h(qr2[0])
+        dag = circuit_to_dag(circ)
+
+        self.assertEqual(dag.depth(), 2)
+
+
 class TestDagEquivalence(QiskitTestCase):
     """DAGCircuit equivalence check."""
 

test/python/transpiler/test_commutation_analysis.py

@@ -21,10 +21,28 @@ class TestCommutationAnalysis(QiskitTestCase):
     """Test the Communttion pass."""
 
     def setUp(self):
-
         self.pass_ = CommutationAnalysis()
         self.pset = self.pass_.property_set = PropertySet()
 
+    def assertCommutationSet(self, result, expected):
+        """ Compares the result of propertyset["commutation_set"] with a dictionary of the form
+        {'q[0]': [ [node_id, ...], [node_id, ...] ]}
+        """
+        result_to_compare = {}
+        for qbit_str, sets in result.items():
+            if not isinstance(qbit_str, str):
+                continue
+            result_to_compare[qbit_str] = []
+            for commutation_set in sets:
+                result_to_compare[qbit_str].append(
+                    sorted([node._node_id for node in commutation_set]))
+
+        for qbit_str, sets in expected.items():
+            for commutation_set in sets:
+                commutation_set.sort()
+
+        self.assertDictEqual(result_to_compare, expected)
+
     def test_commutation_set_property_is_created(self):
         """Test property is created"""
         qr = QuantumRegister(3, 'qr')
@@ -60,9 +78,20 @@ def test_all_gates(self):
         dag = circuit_to_dag(circuit)
 
         self.pass_.run(dag)
-        self.assertEqual(self.pset["commutation_set"]["qr[0]"], [[1], [5], [6], [7], [8, 9, 10, 11],
-                                                                 [12], [13], [14], [15], [16], [2]])
-        self.assertEqual(self.pset["commutation_set"]["qr[1]"], [[3], [14], [15], [16], [4]])
+
+        expected = {'qr[0]': [[1],
+                              [5],
+                              [6],
+                              [7],
+                              [8, 9, 10, 11],
+                              [12],
+                              [13],
+                              [14],
+                              [15],
+                              [16],
+                              [2]],
+                    'qr[1]': [[3], [14], [15], [16], [4]]}
+        self.assertCommutationSet(self.pset["commutation_set"], expected)
 
     def test_non_commutative_circuit(self):
         """A simple circuit where no gates commute
@@ -79,9 +108,9 @@ def test_non_commutative_circuit(self):
         dag = circuit_to_dag(circuit)
 
         self.pass_.run(dag)
-        self.assertEqual(self.pset["commutation_set"]["qr[0]"], [[1], [7], [2]])
-        self.assertEqual(self.pset["commutation_set"]["qr[1]"], [[3], [8], [4]])
-        self.assertEqual(self.pset["commutation_set"]["qr[2]"], [[5], [9], [6]])
+
+        expected = {'qr[0]': [[1], [7], [2]], 'qr[1]': [[3], [8], [4]], 'qr[2]': [[5], [9], [6]]}
+        self.assertCommutationSet(self.pset["commutation_set"], expected)
 
     def test_non_commutative_circuit_2(self):
         """A simple circuit where no gates commute
@@ -100,9 +129,11 @@ def test_non_commutative_circuit_2(self):
         dag = circuit_to_dag(circuit)
 
         self.pass_.run(dag)
-        self.assertEqual(self.pset["commutation_set"]["qr[0]"], [[1], [7], [2]])
-        self.assertEqual(self.pset["commutation_set"]["qr[1]"], [[3], [7], [9], [4]])
-        self.assertEqual(self.pset["commutation_set"]["qr[2]"], [[5], [8], [9], [6]])
+
+        expected = {'qr[0]': [[1], [7], [2]],
+                    'qr[1]': [[3], [7], [9], [4]],
+                    'qr[2]': [[5], [8], [9], [6]]}
+        self.assertCommutationSet(self.pset["commutation_set"], expected)
 
     def test_commutative_circuit(self):
         """A simple circuit where two CNOTs commute
@@ -122,9 +153,11 @@ def test_commutative_circuit(self):
         dag = circuit_to_dag(circuit)
 
         self.pass_.run(dag)
-        self.assertEqual(self.pset["commutation_set"]["qr[0]"], [[1], [7], [2]])
-        self.assertEqual(self.pset["commutation_set"]["qr[1]"], [[3], [7, 9], [4]])
-        self.assertEqual(self.pset["commutation_set"]["qr[2]"], [[5], [8], [9], [6]])
+
+        expected = {'qr[0]': [[1], [7], [2]],
+                    'qr[1]': [[3], [7, 9], [4]],
+                    'qr[2]': [[5], [8], [9], [6]]}
+        self.assertCommutationSet(self.pset["commutation_set"], expected)
 
     def test_commutative_circuit_2(self):
         """A simple circuit where a CNOT and a Z gate commute,
@@ -146,9 +179,11 @@ def test_commutative_circuit_2(self):
         dag = circuit_to_dag(circuit)
 
         self.pass_.run(dag)
-        self.assertEqual(self.pset["commutation_set"]["qr[0]"], [[1], [7, 8], [2]])
-        self.assertEqual(self.pset["commutation_set"]["qr[1]"], [[3], [7, 10], [4]])
-        self.assertEqual(self.pset["commutation_set"]["qr[2]"], [[5], [9], [10], [6]])
+
+        expected = {'qr[0]': [[1], [7, 8], [2]],
+                    'qr[1]': [[3], [7, 10], [4]],
+                    'qr[2]': [[5], [9], [10], [6]]}
+        self.assertCommutationSet(self.pset["commutation_set"], expected)
 
     def test_commutative_circuit_3(self):
         """A simple circuit where multiple gates commute
@@ -169,13 +204,14 @@ def test_commutative_circuit_3(self):
         circuit.x(qr[2])
         circuit.z(qr[0])
         circuit.cx(qr[1], qr[2])
-
         dag = circuit_to_dag(circuit)
 
         self.pass_.run(dag)
-        self.assertEqual(self.pset["commutation_set"]["qr[0]"], [[1], [7, 9, 11, 13], [2]])
-        self.assertEqual(self.pset["commutation_set"]["qr[1]"], [[3], [7, 10, 11], [14], [4]])
-        self.assertEqual(self.pset["commutation_set"]["qr[2]"], [[5], [8], [10], [12, 14], [6]])
+
+        expected = {'qr[0]': [[1], [7, 9, 11, 13], [2]],
+                    'qr[1]': [[3], [7, 10, 11], [14], [4]],
+                    'qr[2]': [[5], [8], [10], [12, 14], [6]]}
+        self.assertCommutationSet(self.pset["commutation_set"], expected)
 
     def test_jordan_wigner_type_circuit(self):
         """A Jordan-Wigner type circuit where consecutive CNOTs commute
@@ -209,12 +245,15 @@ def test_jordan_wigner_type_circuit(self):
         dag = circuit_to_dag(circuit)
 
         self.pass_.run(dag)
-        self.assertEqual(self.pset["commutation_set"]["qr[0]"], [[1], [13, 23], [2]])
-        self.assertEqual(self.pset["commutation_set"]["qr[1]"], [[3], [13], [14, 22], [23], [4]])
-        self.assertEqual(self.pset["commutation_set"]["qr[2]"], [[5], [14], [15, 21], [22], [6]])
-        self.assertEqual(self.pset["commutation_set"]["qr[3]"], [[7], [15], [16, 20], [21], [8]])
-        self.assertEqual(self.pset["commutation_set"]["qr[4]"], [[9], [16], [17, 19], [20], [10]])
-        self.assertEqual(self.pset["commutation_set"]["qr[5]"], [[11], [17], [18], [19], [12]])
+        self.maxDiff = None
+
+        expected = {'qr[0]': [[1], [13, 23], [2]],
+                    'qr[1]': [[3], [13], [14, 22], [23], [4]],
+                    'qr[2]': [[5], [14], [15, 21], [22], [6]],
+                    'qr[3]': [[7], [15], [16, 20], [21], [8]],
+                    'qr[4]': [[9], [16], [17, 19], [20], [10]],
+                    'qr[5]': [[11], [17], [18], [19], [12]]}
+        self.assertCommutationSet(self.pset["commutation_set"], expected)
 
     def test_all_commute_circuit(self):
         """Test circuit with that all commute"""
@@ -233,11 +272,13 @@ def test_all_commute_circuit(self):
         dag = circuit_to_dag(circuit)
 
         self.pass_.run(dag)
-        self.assertEqual(self.pset["commutation_set"]["qr[0]"], [[1], [11, 15, 17], [2]])
-        self.assertEqual(self.pset["commutation_set"]["qr[1]"], [[3], [11, 12, 17, 18], [4]])
-        self.assertEqual(self.pset["commutation_set"]["qr[2]"], [[5], [12, 14, 18, 20], [6]])
-        self.assertEqual(self.pset["commutation_set"]["qr[3]"], [[7], [13, 14, 19, 20], [8]])
-        self.assertEqual(self.pset["commutation_set"]["qr[4]"], [[9], [13, 16, 19], [10]])
+
+        expected = {'qr[0]': [[1], [11, 15, 17], [2]],
+                    'qr[1]': [[3], [11, 12, 17, 18], [4]],
+                    'qr[2]': [[5], [12, 14, 18, 20], [6]],
+                    'qr[3]': [[7], [13, 14, 19, 20], [8]],
+                    'qr[4]': [[9], [13, 16, 19], [10]]}
+        self.assertCommutationSet(self.pset["commutation_set"], expected)
 
 
 if __name__ == '__main__':