-
Notifications
You must be signed in to change notification settings - Fork 309
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
nx-cugraph: add weakly connected components #4071
Conversation
So, I suppose this goes to your definition of weakly connected components. Networkx defines this as you describe:
A quick scan of the literature suggests that while this is perhaps a good labeling (matches Knuth, always a plus in my mind), that it's hardly universal. Our implementation at the C++ level computes We don't really have an efficient way of traversing the edges backwards from whatever orientation that we have. Our CSR/DCSR representation only efficiently reflects one direction. We have several options we can pursue:
I'm leaning toward option 3, but am open to other options. |
Thanks for the thoughtful and helpful reply @ChuckHastings! I went ahead with option 1 where we symmetrize in Python before creating the PLC graph. This is probably good enough for a while. Nevertheless, I did it in a way that will let us easily switch to options 2 or 3 if/when available. I chose the keyword argument Related topic: is SCC on your radar to do any time soon? I'm less familiar with SCC algorithms and literature, but maybe https://doi.org/10.14778/2733085.2733089 |
We have a low priority activity exploring SCC. Constructing a good algorithm for the GPU is complicated as the best serial algorithms use depth-first search which doesn't parallelize well. I wouldn't expect a good implementation this year unless we find some customer that urgently wants it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I did point out something this PR might need from this PR that might make it worth waiting for and updating.
python/nx-cugraph/nx_cugraph/algorithms/components/connected.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I had one question below which need not hold up approval.
def number_strongly_connected_components(G): | ||
G = _to_directed_graph(G) | ||
if G.src_indices.size == 0: | ||
return len(G) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just curious, no action needed: is this expected to always return 0 in this case? If so, is there a reason calling len()
is preferred over just returning 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I should use G.number_of_edges()
instead of G.src_indices.size
(but for some reason the latter is easier for me to remember and reason about). Anyway, if the number of edges are zero, the the number of components is the number of nodes, hence we can't simply return 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may update to use number_of_edges
lots of places for clarity in a different PR. I agree this shouldn't hold up this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, number_of_edges
actually does a lot more work. If we want to know if there are exactly 0 edges, G.src_indices.size
works great.
/merge |
NetworkX tests are somewhat underspecified regarding how to handle self-loops for these algorithms. Also, I'm not sure if transitivity is supposed to work on directed graphs. Once #4071 is merged, it should be easy to add `is_bipartite` function (and maybe others?). Authors: - Erik Welch (https://github.com/eriknw) Approvers: - Rick Ratzel (https://github.com/rlratzel) URL: #4093
This doesn't currently work, because
plc.weakly_connected_components
only works on symmetric graphs (so it's not actually performing wcc now is it?):These are high-priority algorithms for
nx-cugraph
, because they are widely used by networkx dependents.