Add substitute_node_with_subgraph method to PyDiGraph

[mtreinish]

This commit adds a new method substitute_node_with_subgraph that takes
in a node id and a PyDiGraph object and then procedes to replace the
specified node with the subgraph.

TODO:

• Add tests
• Add release notes
• Verify functionality for use with qiskit-terra's substitute_node_with_dag

[coveralls]

Pull Request Test Coverage Report for Build 968084539

• 123 of 124 (99.19%) changed or added relevant lines in 3 files are covered.
• 2 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+0.001%) to 97.516%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/digraph.rs	99	100	99.0%

Files with Coverage Reduction	New Missed Lines	%
src/isomorphism.rs	2	97.51%

Totals
Change from base Build 968015678:	0.001%
Covered Lines:	8285
Relevant Lines:	8496

---

💛 - Coveralls

[mtreinish]

Ok, I've updated this and I think it's ready for review now. There is a matching terra PR which shows this in use: Qiskit/qiskit#6302

[nahumsa]

I think that the node index ordering of the substituted graph needs to be explicit in the documentation or maybe reindexing all nodes of the new graph.

For instance, I have the original_graph being

and the other_graph being

using substitute_node_with_subgraph on node 2, we get

All new nodes start at the max value of the 'original_graph` node index, but this is not explicit in the documentation. Or maybe it could be useful to reindexing all nodes or even giving the choice for the user to choose the index of the added nodes.

Here is the code to reproduce what I said:

import retworkx
from retworkx.visualization import mpl_draw
original_graph = retworkx.generators.directed_path_graph(5)
other_graph = retworkx.generators.directed_path_graph(2)
original_graph.substitute_node_with_subgraph(2, other_graph, lambda _, __, ___: 0)
mpl_draw(original_graph, with_labels=True)

This concerns me because if I add a node on the graph after this, the node added will have index 2 and I would expect index to be 7.

original_graph.add_parent(6, None, None)
mpl_draw(original_graph, with_labels=True)

[mtreinish]

This is unavoidable because we can't preserve the indices between the 2 graphs as there might be overlap and we're essentially just calling add_node() for each node in the graph. But this is why the return from the function is a custom mapping class of old node indices in other_graph to it's new index in original_graph so you can run this and then use that dictionary I will try to improve the documentation around this to make this clear.

For the second half I'm actually not actually sure how node index 2 is getting added there. I agree it should be 7, let me dig into that with your example locally and see if I can figure out what's going on

[mtreinish]

So I dug into this it's a behavior of petgraph's stable graph. It will reuse node indices after removal:

https://github.com/petgraph/petgraph/blob/0.5.1/src/graph_impl/stable_graph/mod.rs#L246-L253

I was able to reproduce this manually with:

graph = retworkx.PyDiGraph()
graph.add_nodes_from(list(range(5)))
graph.add_nodes_from(list(range(2)))
graph.remove_node(2)
res_manual = graph.add_parent(6, None, None)
print(res_manual)

which returns 2. I'm guessing it does this to preserve memory. But, this caught me by surprise too, but then I haven't manually inspected the indices post removal like this before. When I've been using graphs with removals I been interacting with the indices programmatically and never looked at them directly. I think we should definitely document this in the class docstrings for PyGraph, PyDiGraph, PyDAG because it definitely isn't straightforward I will open an issue about this.

[nahumsa]

This is unavoidable because we can't preserve the indices between the 2 graphs as there might be overlap and we're essentially just calling add_node() for each node in the graph. But this is why the return from the function is a custom mapping class of old node indices in other_graph to it's new index in original_graph so you can run this and then use that dictionary I will try to improve the documentation around this to make this clear.

Sure, that makes sense. I forgot about the return, sorry about that.

[1ucian0]

I think edge_map_fn should be optional.

[mtreinish]

This is required right now because we without it we don't know what to do with edges to or from node. If we switched it to optional what do you think the default behavior should it was not set? The only option I could think of is to just drop all the edges into or out of node if this wasn't set, but if we did this it wouldn't really feel like it was substituting the node with a graph.

[1ucian0]

Is this a typo?

Suggested change

	#[text_signature = "(self, node, other, edge_map_fn, /, node_filter=None, edge_weight_map=None)"]
	#[text_signature = "(self, node, other, edge_map_fn, node_filter=None, edge_weight_map=None)"]

[mtreinish]

It's actually intentional, the / indicates the end of positional only arguments (it's part of the cPython interface and is actually a valid python syntax for >=3.8). See the docs for the rust python interface here: https://pyo3.rs/v0.9.2/function.html#making-the-function-signature-available-to-python which explains this.