retain adjacency order in nx-loopback copy of networkx graph

[dschult]

This rewrites the copy functionality within nx-loopback to ensure that the neighbor order is the same in the copy. Instead of copying edges, this copies adjacencies (careful to ensure opposite representations of an edge point to the same datadict).

The result is that the incoming PR for fixing colliders and v-structures passes its tests.
Also, dfs_labeled_edges test passes. In fact all the topological sort functions that were excluded pass the tests as well, though I'm not sure whether we should still exclude them from the nx-loopback tests because I don't know what the issue was regarding the function changing the original graph. E.g. maybe we shouldn't be copying the graph at all since it needs to be changed.

~~This also seems to fix the long time for the nx-loopback tests. It is still about 8 secs (~10-15%) longer to run, but not multiple minutes. Almost all of that is due to using G._adj directly instead of add_edges_from and G.edges.~~ [Edit: error on my part caused mistaken speed results. With the change it is the same speed as it was.]

@eriknw can you look at my comments near the top of the diff:

1. should any of these functions marked as being skipped be included? (I think the topo sort maybe) [A: Only the dfs_labeled_edges is fixed by the order of edges in loopback. The rest stay in place.]
2. the conversion of an AntiGraph to a normal graph class is just G = nx.complement(A). But I'm not sure how that helps the code near the comment about AntiGraph. [A: Removing comment about AntiGraph]

[dschult]

If this copy code works for nx-loopback -- I think we should update the copy methods in the base classes to ensure that the neighbor/adjacency order is maintained when users do G.copy(). It didn't matter when the order of the neighbors was arbitrary based on storage/reporting within a Python dict. But now that dicts have ordered keys, users might expect the order to be the same after a copy.

Would maintaining the adjacency order during copy be considered a breaking change? We didn't tell people the current reordering was a feature -- indeed, we said the order could be machine dependent. But it is possible that some people are counting on a specific order after a copy. Still, my inclination is that implementing copy as maintaining neighbor order is not breaking any assurance. Your output may change from what it was, but now it has a defined pattern. Anyway, I'm open to a deprecation on that (still to be written) PR if/when we get to it.

[eriknw]

Very nice!

Using a similar approach for G.copy to maintain order makes sense to me--just be sure to add a remark to the next release notes.

This also seems to fix the long time for the nx-loopback tests. It is still about 8 secs (~10-15%) longer to run, but not multiple minutes.

Where do you see this? "dispatch" CI still takes several minutes. I have not experimented with this locally yet.

[rossbar]

Here's a naive question: does the loopback interface need to explicitly copy at all? In principle, the dispatching is handled by looking up the __networkx_backend__ attr on the instance, so can't that just be added to the input graph instance without any copying? In other words, the convert_from_nx function might look something like:

def convert_from_nx(graph, *, ...):
    graph.__networkx_backend__ = "nx-loopback"
    return graph

I guess the main issue is that this wouldn't probe the kwarg values of convert_from_nx... is that right? Are those probed now (i.e. taken from the @_dispatch decorator values)?

---

On the copy question: +1 from me for refactoring copy to preserve ordering - that makes a lot of sense as of Python 3.7!

[dschult]

The timing results I was getting are apparently a "feature" of my local testing environment. I would like some help in getting my environment set up better.

Running NETWORKX_TEST_BACKEND=nx-loopback pytest --doctest-modules --durations=10 --pyargs networkx from the base dir of my repo runs all the tests just fine without the loopback!! I get the same results as running it without the environment variable set. I can add an assert False to a test file in the repo and that test fails. So I am testing the correct files, just not with the loopback backend. I have put assert False into the loopback.convert_from_nx function and it is never reached.

NETWORKX_TEST_BACKEND=nx-loopback pytest --doctest-modules --durations=10 --pyargs networkx/classes (or any other specific file or dir within the networkx repo) runs with the nx-loopback process. All the files and loopback functions are called as they should be.

I do not have networkx installed in this mamba environment. I have used pip install -e . to create the egg directory in the repo's home directory. Without that, the testing command leads to an error about nx-loopback not being a backend. And that error happens even when pointing to specific files/dirs to run the tests for.

So... How do i run pytest on the whole repo using nx-loopback as my backend?

[dschult]

I've now found that I can get nx-loopback testing feature working while testing the whole library so long as I don't try to use the --doctest-modules option. :) Both of these work:
NETWORKX_TEST_BACKEND=nx-loopback pytest --pyargs networkx and
NETWORKX_TEST_BACKEND=nx-loopback pytest networkx

while adding --doctest-modules does not convert the graph:
NETWORKX_TEST_BACKEND=nx-loopback pytest --doctest-modules --pyargs networkx

If I specify a file/directory within networkx it works with or without --doctest-modules. I am trying to mimic the environment created by the github "dispatch" workflow -- but I'm on mac...

[Found the trouble. I had an unstaged python file that set backend information in the repo's directory. When using --docutils-modules that file was executed (which I did not expect... I figured it would just be searched for doc_strings.). Removing that file (starting with a clean repo and no staged files) resolved the issue.]

[dschult]

This change to keep edge order consistent during nx-loopback conversion is ready for review.
It should work to make #7398 pass the dispatch tests -- but I haven't tested that yet.

[Edit: Yes -- this allows the nx-loopback tests in #7398 pass. :) ]

[rossbar]

LGTM, thanks @dschult !

[rossbar]

It looks like "adj" and "succ" could just be added to this list to save a few lines, but there's no reason to do so other than style. Just thought I'd mention!

[dschult]

Thanks for those comments and suggestions @rossbar.
I have made the suggested changes. And @eriknw has reviewed the PR with comments/suggestions above.
So I think it is ready to merge -- but I'd feel more confident of that after Erik has a chance to look again (and github approve it?).

[eriknw]

LGTM, this is pretty slick! It took me a little while to convince myself that inner/outer data dicts were handled correctly for multigraphs, but it's straightforward enough. The comments were helpful regarding ensuring that certain dicts be the same.