Use retworkx for substitute_node_with_dag

[mtreinish]

Summary

This commit leverage the substitute_node_with_subgraph method being
added Qiskit/rustworkx#312 for the dagcircuit method
substitute_node_with_dag.

Details and comments

TODO:

• Fix test failures (~39 failures looks like all related to controlled gates)
• Wait for Add substitute_node_with_subgraph method to PyDiGraph rustworkx#312 to merge and release
• Benchmark after Use symengine for parameter and parameter expressions #6270 (with symengine circuits like a 7 qubit qv are bottlenecked on calls to substitute_node_with_dag, prior to this sympy takes up ~35-50% of the profile and it's too difficult to see differences from anything else)

[mtreinish]

I ran some simple benchmarks with this script:

import time
import statistics
from qiskit.circuit.random import random_circuit
from qiskit.test.mock import FakeMontreal
from qiskit import transpile
backend = FakeMontreal()
qc = random_circuit(27, 100, measure=True, seed=42)
transpile(qc, backend)
times = []      
for i in range(10):
    start = time.time()
    transpile(qc, backend)
    stop = time.time()
    times.append(stop - start)
print(statistics.mean(times))

On main without this PR this returned 7.581698060035706 seconds and with this PR it returned 6.584884548187256 seconds.

Then jumping it up to optimization_level=3 on the transpile calls (but changing to range(3) in the interest of time) on main the script returned 285.9030789534251 seconds and with this PR it returned 189.92249496777853 seconds

[mtreinish]

Now that retworkx 0.10.x has been released this is unblocked

[jakelishman]

This code and tests all look correct to me, so I'm happy enough to approve this. I did leave a couple of questions that are about my understanding of the DAGCircuit, though.

[jakelishman]

To check, here you're just using the second argument of lambda edge, wire=self_wire: edge == wire purely for the normal safe scoping within the lambda, right? e.g. it's only ever intended to be called with one argument, like self_wire.__eq__ but with conversion of NotImplemented to errors.

[mtreinish]

Yeah, I did this solely for scoping. IIRC when I wrote this originally I didn't do this and lint or something complained. The filter_fn argument only gets passed a single arg which is the weight/data payload object for each edge: https://qiskit.org/documentation/retworkx/dev/stubs/retworkx.PyDiGraph.find_predecessors_by_edge.html#retworkx.PyDiGraph.find_predecessors_by_edge

[jakelishman]

This looks correct, but I feel like I'm potentially missing something big in the implementation of DAGCircuit; I would have expected that the call self._multi_graph.substitute_node_with_subgraph would have replaced the nodes already. Or is this bit just about updating the references to Bit objects in the nodes afterwards?

[mtreinish]

Yeah, this is just house keeping for the Bit objects in each DAG node object.

graph.substitute_node_with_subgraph(node, other, edge_map_fn) copies the nodes and edges from other into graph, deletes node in graph and wires things into and out of node as described by edge_map_fn. But the nodes in other still have the same weight/data payload from before the migration. So this section is just about updating the qargs and cargs in the nodes to be the bits from graph instead of the bits in other.

[jakelishman]

Also, I think making this "Changelog: None" is doing yourself and the others on retworkx a bit of a disservice - it's a significant performance improvement, and it wouldn't be a bad thing to mention it in the notes!

[mtreinish]

Also, I think making this "Changelog: None" is doing yourself and the others on retworkx a bit of a disservice - it's a significant performance improvement, and it wouldn't be a bad thing to mention it in the notes!

Heh, I wasn't sure exactly how to describe it in the release notes or even which changelog label would apply as it's not really a user facing change. I'd gladly advertise it in either place but wasn't sure how to word it so it was meaningful for users reading the release documentation.

[kdk]

A few suggestions on comments/readability, but otherwise, looks good to me.

[jakelishman]

I've added a reno mentioning the performance benefits so Matthew doesn't have to toot his own horn - I've stuck it in "features", since I think performance is one of our features.

Sticking my approval on it so it can be merged if someone else is happy with it, but feel free to review the reno/suggest its removal.