Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Change degree_type of core_number to "outgoing" #28

Merged
merged 2 commits into from
Nov 20, 2024
Merged

Change degree_type of core_number to "outgoing" #28

merged 2 commits into from
Nov 20, 2024

Conversation

eriknw
Copy link
Contributor

@eriknw eriknw commented Nov 16, 2024

No description provided.

@eriknw eriknw added non-breaking Introduces a non-breaking change improvement Improves an existing functionality labels Nov 16, 2024
@eriknw eriknw added this to the 24.12 milestone Nov 19, 2024
@rlratzel
Copy link
Contributor

rlratzel commented Nov 19, 2024 •
edited
Loading

Can you please provide a description for this PR to explain why we need the change? (I'm assuming this would be a summary of the offline conversation with @ChuckHastings ). Can you also update the PLC docstring here too? (oops, forgot this is a separate repo, I'll fix the docstring in a separate cugraph PR)

@eriknw
Copy link
Contributor Author

eriknw commented Nov 19, 2024

degree_type="bidirectional" was double-counting as of rapidsai/cugraph#4755, but degree_type="outgoing" and degree_type="incoming" do not. We had added a hotfix in #26 that divides by 2 to unblock CI. Choosing a more appropriate degree_type seems like the better fix.

I suppose degree_type="bidirectional" is meant for when you want to treat e.g. directed graph with half the edges (such as a having a triangular adjacency matrix) as an undirected graph.

Can you also update the PLC docstring here too? (oops, forgot this is a separate repo, I'll fix the docstring in a separate cugraph PR)

Thanks! I'm not sure I understand the options well enough to describe them usefully to end users.

eriknw
eriknw commented Nov 20, 2024
View reviewed changes
nx_cugraph/algorithms/core.py
@@ -43,10 +43,9 @@ def core_number(G):
node_ids, core_numbers = plc.core_number(
resource_handle=plc.ResourceHandle(),
graph=G._get_plc_graph(),
degree_type="bidirectional",
degree_type="outgoing",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
degree_type="outgoing",
degree_type="incoming",

@rlratzel rlratzel self-requested a review November 20, 2024 21:30
@rlratzel
Copy link
Contributor

/merge

@rapids-bot rapids-bot bot merged commit 8a6e223 into rapidsai:branch-24.12 Nov 20, 2024
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rlratzel rlratzel rlratzel approved these changes

Assignees
No one assigned
Labels
improvement Improves an existing functionality non-breaking Introduces a non-breaking change
Projects
None yet
Milestone
24.12
Development

Successfully merging this pull request may close these issues.
2 participants
@eriknw @rlratzel