-
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 CC for undirected graphs to fix k-truss #3965
Conversation
@number_connected_components._can_run | ||
def _(G): | ||
# NetworkX <= 3.2.1 does not check directedness for us | ||
try: | ||
return not G.is_directed() | ||
except Exception: | ||
return False |
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 fixed this upstream here: networkx/networkx#7074
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.
changes LGTM. Should there be a test for k_truss that passes
G=nx.Graph([(0,1), (1,2), (2,0), (3,4), (4,5), (5,3)])
and asserts that NotImplementedError
is raised, which we then xfail
when k_truss is updated?
OTOH, maybe that's overkill for (hopefully) temporary code that I'm assuming was at least manually tested? I'm fine either way.
Thanks for providing a test case, but now we actually have the opposite problem: now we don't test the main branches of k_truss! All test graphs apparently have more than 1 connected components. |
Karate club has only 1 CC; maybe add a quick test for that too to make sure we at least exercise |
Excellent suggestion, although we'd need #3954 to test with karate club (b/c it has string node values). Test added, and code is exercised 👍 I'm looking forward to having more graph generators to enable more comparison testing of algorithms. |
def _groupby( | ||
groups: cp.ndarray, values: cp.ndarray, groups_are_canonical: bool = False | ||
) -> dict[int, cp.ndarray]: |
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'm really happy with this groupby implementation btw. Using prepend
with cp.diff
is a little slow compared to cp.diff
w/o it, but this isn't where the bulk of the time is spent.
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.
Thanks for adding the test.
/merge |
Fixes #3963 and add
connected_components
,is_connected
,node_connected_component
, andnumber_connected_components
.Also updated
_groupby
to handle groups that are not consecutive integers starting with 0.Also,
plc.weakly_connected_components
does not handle isolated nodes well, and I needed to handle this at the Python layer as was done in #3897