Add collect_runs() function

[mtreinish]

This commit adds a new algorithms function, collect_runs(), which is
used to find a list of runs in a PyDiGraph object that match a specified
condition. A run in this context is any linear path inside the graph.
The condition is specified using a python function that will be passed a
node's data payload/weight object and will return whether it matches the
condition or not. The intended use case for this function is with
qiskit-terra's PyDiGraph method collect_runs() which is implementing
this algorithm currently in Python on top of PyDiGraph (it will also be
used by a future method collect_1q_runs which is being added in
Qiskit/qiskit#5431).

TODO

• Add tests

[mtreinish]

Marked as on hold because I still need to write tests for the new function. I tested this manually by updating terra to use the new function instead of it's current implementation and it worked fine, but we need more direct test coverage here

[coveralls]

Pull Request Test Coverage Report for Build 396952472

• 34 of 34 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.06%) to 94.784%

---

Totals
Change from base Build 396951846:	0.06%
Covered Lines:	3053
Relevant Lines:	3221

---

💛 - Coveralls

[mtreinish]

I think there are also some edge cases here when I ran qiskit-terra's tests with:

index d56ca28d..a1480a5b 100644
--- a/qiskit/dagcircuit/dagcircuit.py
+++ b/qiskit/dagcircuit/dagcircuit.py
@@ -1378,29 +1378,13 @@ class DAGCircuit:
 
         Nodes must have only one successor to continue the run.
         """
-        group_list = []
-
-        # Iterate through the nodes of self in topological order
-        # and form tuples containing sequences of gates
-        # on the same qubit(s).
-        topo_ops = list(self.topological_op_nodes())
-        nodes_seen = dict(zip(topo_ops, [False] * len(topo_ops)))
-        for node in topo_ops:
-            if node.name in namelist and node.condition is None \
-                    and not nodes_seen[node]:
-                group = [node]
-                nodes_seen[node] = True
-                s = self._multi_graph.successors(node._node_id)
-                while len(s) == 1 and \
-                        s[0].type == "op" and \
-                        s[0].name in namelist and \
-                        s[0].condition is None:
-                    group.append(s[0])
-                    nodes_seen[s[0]] = True
-                    s = self._multi_graph.successors(s[0]._node_id)
-                if len(group) >= 1:
-                    group_list.append(tuple(group))
-        return set(group_list)
+
+        def filter_fn(node):
+            return node.type == "op" and node.name in namelist and node.condition is None
+
+        group_list = rx.collect_runs(self._multi_graph, filter_fn)
+        print(group_list)
+        return set(tuple(x) for x in group_list)
 
     def nodes_on_wire(self, wire, only_ops=False):
         """

applied most of the tests work, but there are still some failures caused by differences in the returned runs between this function and what was in terra before.

[mtreinish]

Ok, I fixed the bug the terra unit tests identified in: 4c5d8eb and also added some initial tests in be9c548 I'll probably add a couple more negative tests depending on what coveralls shows, but this should be ready to go now.

[itoko]

I understand what is strange for the original implementation in qiskit-terra.
The original one can report a node in multiple runs.
For example, it reports cx twice, {('h', 'cx'), ('h', 'cx')}, for the circuit below.

     ┌───┐     
q_0: ┤ H ├──■──
     ├───┤┌─┴─┐
q_1: ┤ H ├┤ X ├
     └───┘└───┘

The collect_run() implemented in this PR prevents such a duplication, and reports
[['h', 'cx'], ['h']] for the above case.

I confirmed that this change does not affect to current DAGCircuit usages within terra, collect_run is only used to collect consecutive CNOTs or 1q-gates. So I have little concern on the spec change induced by this PR.

I like the spec implemented in this PR (it's clearer), so I suggest to

• clearly state the point above (no duplication) in the docstring, e.g. Each node can belong to at most one run.
• add the above case to the test (and two more as below, if you don't mind)

    def test_h_h_cx(self):
        dag = retworkx.PyDiGraph()
        q0, q1 = dag.add_nodes_from(['q0', 'q1'])
        h_1 = dag.add_child(q0, 'h', 'q0')
        h_2 = dag.add_child(q1, 'h', 'q1')
        cx_2 = dag.add_child(h_1, 'cx', 'q0')
        dag.add_edge(h_2, cx_2, 'q1')

        def filter_function(node):
            return node in ['cx', 'h']

        res = retworkx.collect_runs(dag, filter_function)
        self.assertEqual([['h', 'cx'], ['h']], res)

    def test_cx_h_h_cx(self):
        dag = retworkx.PyDiGraph()
        q0, q1 = dag.add_nodes_from(['q0', 'q1'])
        cx_1 = dag.add_child(q0, 'cx', 'q0')
        dag.add_edge(q1, cx_1, 'q1')
        h_1 = dag.add_child(cx_1, 'h', 'q0')
        h_2 = dag.add_child(cx_1, 'h', 'q1')
        cx_2 = dag.add_child(h_1, 'cx', 'q0')
        dag.add_edge(h_2, cx_2, 'q1')

        def filter_function(node):
            return node in ['cx', 'h']

        res = retworkx.collect_runs(dag, filter_function)
        self.assertEqual([['cx'], ['h', 'cx'], ['h']], res)

    def test_cx_h_cx(self):
        dag = retworkx.PyDiGraph()
        q0, q1 = dag.add_nodes_from(['q0', 'q1'])
        cx_1 = dag.add_child(q0, 'cx', 'q0')
        dag.add_edge(q1, cx_1, 'q1')
        h_1 = dag.add_child(cx_1, 'h', 'q0')
        cx_2 = dag.add_child(h_1, 'cx', 'q0')
        dag.add_edge(cx_1, cx_2, 'q1')

        def filter_function(node):
            return node in ['cx', 'h']

        res = retworkx.collect_runs(dag, filter_function)
        self.assertEqual([['cx'], ['h', 'cx']], res)

The above cases correspond with the circuits, respectively:

     ┌───┐     
q_0: ┤ H ├──■──
     ├───┤┌─┴─┐
q_1: ┤ H ├┤ X ├
     └───┘└───┘

          ┌───┐     
q_0: ──■──┤ H ├──■──
     ┌─┴─┐├───┤┌─┴─┐
q_1: ┤ X ├┤ H ├┤ X ├
     └───┘└───┘└───┘

          ┌───┐     
q_0: ──■──┤ H ├──■──
     ┌─┴─┐└───┘┌─┴─┐
q_1: ┤ X ├─────┤ X ├
     └───┘     └───┘

I'm not sure adding circuit diagrams in docstring helps readers as they don't have information on edge directions.

[mtreinish]

I understand what is strange for the original implementation in qiskit-terra.
The original one can report a node in multiple runs.

Oh, that's interesting, I'm actually a bit surprised I basically just ported the logic from the terra DAGCircuit.collect_runs() method to rust and adapted it to work inside retworkx. I didn't notice this, but also didn't think to test it since the use cases I had in mind were all about the getting the 1q gates runs for the optimize_1q_gates passes.

For example, it reports cx twice, {('h', 'cx'), ('h', 'cx')}, for the circuit below.

     ┌───┐     
q_0: ┤ H ├──■──
     ├───┤┌─┴─┐
q_1: ┤ H ├┤ X ├
     └───┘└───┘

The collect_run() implemented in this PR prevents such a duplication, and reports
[['h', 'cx'], ['h']] for the above case.

I confirmed that this change does not affect to current DAGCircuit usages within terra, collect_run is only used to collect consecutive CNOTs or 1q-gates. So I have little concern on the spec change induced by this PR.

I like the spec implemented in this PR (it's clearer), so I suggest to

* clearly state the point above (no duplication) in the docstring, e.g. Each node can belong to at most one run.

* add the above case to the test (and two more as below, if you don't mind)

    def test_h_h_cx(self):
        dag = retworkx.PyDiGraph()
        q0, q1 = dag.add_nodes_from(['q0', 'q1'])
        h_1 = dag.add_child(q0, 'h', 'q0')
        h_2 = dag.add_child(q1, 'h', 'q1')
        cx_2 = dag.add_child(h_1, 'cx', 'q0')
        dag.add_edge(h_2, cx_2, 'q1')

        def filter_function(node):
            return node in ['cx', 'h']

        res = retworkx.collect_runs(dag, filter_function)
        self.assertEqual([['h', 'cx'], ['h']], res)

    def test_cx_h_h_cx(self):
        dag = retworkx.PyDiGraph()
        q0, q1 = dag.add_nodes_from(['q0', 'q1'])
        cx_1 = dag.add_child(q0, 'cx', 'q0')
        dag.add_edge(q1, cx_1, 'q1')
        h_1 = dag.add_child(cx_1, 'h', 'q0')
        h_2 = dag.add_child(cx_1, 'h', 'q1')
        cx_2 = dag.add_child(h_1, 'cx', 'q0')
        dag.add_edge(h_2, cx_2, 'q1')

        def filter_function(node):
            return node in ['cx', 'h']

        res = retworkx.collect_runs(dag, filter_function)
        self.assertEqual([['cx'], ['h', 'cx'], ['h']], res)

    def test_cx_h_cx(self):
        dag = retworkx.PyDiGraph()
        q0, q1 = dag.add_nodes_from(['q0', 'q1'])
        cx_1 = dag.add_child(q0, 'cx', 'q0')
        dag.add_edge(q1, cx_1, 'q1')
        h_1 = dag.add_child(cx_1, 'h', 'q0')
        cx_2 = dag.add_child(h_1, 'cx', 'q0')
        dag.add_edge(cx_1, cx_2, 'q1')

        def filter_function(node):
            return node in ['cx', 'h']

        res = retworkx.collect_runs(dag, filter_function)
        self.assertEqual([['cx'], ['h', 'cx']], res)

Yeah I agree with this, I've done this in: becd668

The above cases correspond with the circuits, respectively:

     ┌───┐     
q_0: ┤ H ├──■──
     ├───┤┌─┴─┐
q_1: ┤ H ├┤ X ├
     └───┘└───┘

          ┌───┐     
q_0: ──■──┤ H ├──■──
     ┌─┴─┐├───┤┌─┴─┐
q_1: ┤ X ├┤ H ├┤ X ├
     └───┘└───┘└───┘

          ┌───┐     
q_0: ──■──┤ H ├──■──
     ┌─┴─┐└───┘┌─┴─┐
q_1: ┤ X ├─────┤ X ├
     └───┘     └───┘

I'm not sure adding circuit diagrams in docstring helps readers as they don't have information on edge directions.

Yeah I'm not sure if circuit diagrams make sense in the docstring for retworkx tests, I can add them if you think it makes sense. For retworkx functions we can always leverage the graph drawer and jupyter-execute like: https://github.com/Qiskit/retworkx/blob/master/src/generators.rs#L50-L71 That doesn't do anything for tests though.