nx-cugraph: add relabel_nodes and convert_node_labels_to_integers
#4531
Conversation
eriknw
added
improvement
| key( | ||
| "test_relabel.py:" | ||
| "test_relabel_preserve_node_order_partial_mapping_with_copy_false" | ||
| ): "Node order is preserved when relabeling with partial mapping", |
There was a problem hiding this comment.
I don't fully grok why this test is checking that the node order changed. It seems like we'd want to keep the node order the same (and we do). See:
eriknw
changed the title
relabel_nodesrelabel_nodes and convert_node_labels_to_integers
There was a problem hiding this comment.
Thanks, glad to see the update to handle copy=False for NX graphs, and also glad to see the updates for NX 3.4 (we should also add test runs using NX from its main branch to our nightlies, cc @nv-rliu ).
It was a bit hard to review though since relabel_nodes is a dense ~180 lines long :) It's also too bad there's so many new xfails, but I guess that's understandable.
Also, I'm very curious what the speedup over NX will be, especially if they pass in a NX graph. Do you have any benchmark results to share?
|
/merge |
| @generic_bfs_edges._can_run | ||
| def _(G, source, neighbors=None, depth_limit=None, sort_neighbors=None): | ||
| return neighbors is None and sort_neighbors is None | ||
| if nx.__version__[:3] <= "3.3": |
There was a problem hiding this comment.
String comparison isn’t appropriate here (breaks with versions like 3.30). Try comparing version specifiers like this:
from packaging.version import parse
if parse(nx.__version__) <= parse("3.3"):
…There was a problem hiding this comment.
Here's an alternative solution: #4571
rapids-bot
bot
merged commit aa0347c
into
rapidsai:branch-24.08
This is to address the feedback from @bdice in #4531 (comment), which I was unable to do in that PR. I know checking version strings is playing it fast-and-loose, but I believe we are abundantly safe to do so here, as networkx releases have been, and are expected to be, slow and predictable. Also, we do not have `packaging` as a runtime dependency (it _is_ a test dependency for finer control). So, even better than using `packaging.version.parse`, this PR performs a sanity check on the networkx version. I put it in both `nx_cugraph.__init__` and `_nx_cugraph.__init__` to give us obvious breadcrumbs to discover and follow. Authors: - Erik Welch (https://github.com/eriknw) Approvers: - Rick Ratzel (https://github.com/rlratzel) URL: #4571
This was pretty tricky in places! This begins with
GraphandDiGraph, which are probably tricker, because of the way edge data get merged when nodes are combined (and I wouldn't be surprised if there are other ways to do this operation).This is one of the most heavily used functions in networkx and also networkx dependents.
Up next: multigraphs