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

Fix Leiden refinement phase #3990

Merged
merged 21 commits into from
Nov 20, 2023
Merged

Fix Leiden refinement phase #3990

merged 21 commits into from
Nov 20, 2023

Conversation

naimnv
Copy link
Contributor

@naimnv naimnv commented Nov 8, 2023 •
edited
Loading

  • Normalization factor was missing in the equation to decide if a node and a refined community is strongly connected inside their Louvain community. This PR adds that factor.
  • Disable random moves in the refinement phase. We plan to expose a flag to enable/disable random moves in a future PR.
  • Adds new function to flatten Leiden dendrogram as dendrogram flattening process needs additional info to unroll hierarchical leiden clustering

Closes #3850
Closes #3749

@naimnv naimnv requested a review from a team as a code owner November 8, 2023 17:44
@copy-pr-bot copy-pr-bot
Copy link

copy-pr-bot bot commented Nov 8, 2023

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@naimnv naimnv added bug Something isn't working non-breaking Non-breaking change labels Nov 8, 2023
@naimnv naimnv self-assigned this Nov 8, 2023
@naimnv
Copy link
Contributor Author

naimnv commented Nov 8, 2023

/ok to test

seunghwak
seunghwak approved these changes Nov 8, 2023
View reviewed changes
Copy link
Contributor

@seunghwak seunghwak left a comment

Choose a reason for hiding this comment

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

LGTM but please use FIXME to better track issues we need to revisit.

cpp/src/community/detail/common_methods.cuh Outdated Show resolved Hide resolved
cpp/src/community/detail/refine_impl.cuh Outdated Show resolved Hide resolved
@naimnv
Copy link
Contributor Author

naimnv commented Nov 8, 2023

/ok to test

@alexbarghi-nv
Copy link
Member

/ok to test

@BradReesWork BradReesWork added this to the 23.12 milestone Nov 14, 2023
@ChuckHastings
Copy link
Collaborator

/ok to test

@naimnv
Copy link
Contributor Author

naimnv commented Nov 16, 2023

/ok to test

@naimnv naimnv requested a review from a team as a code owner November 16, 2023 15:28
@naimnv
Copy link
Contributor Author

naimnv commented Nov 16, 2023

/ok to test

@naimnv
Copy link
Contributor Author

naimnv commented Nov 16, 2023

/ok to test

@naimnv
Copy link
Contributor Author

naimnv commented Nov 16, 2023

/ok to test

@naimnv
Copy link
Contributor Author

naimnv commented Nov 17, 2023

/ok to test

@naimnv
Copy link
Contributor Author

naimnv commented Nov 17, 2023

/ok to test

@naimnv
Copy link
Contributor Author

naimnv commented Nov 17, 2023

/ok to test

@naimnv
Copy link
Contributor Author

naimnv commented Nov 17, 2023

/ok to test

@naimnv
Copy link
Contributor Author

naimnv commented Nov 17, 2023

/ok to test

@naimnv
Copy link
Contributor Author

naimnv commented Nov 18, 2023

/ok to test

@ChuckHastings
Copy link
Collaborator

/merge

@rapids-bot rapids-bot bot merged commit 8549b54 into rapidsai:branch-23.12 Nov 20, 2023
74 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BradReesWork BradReesWork BradReesWork approved these changes

@ChuckHastings ChuckHastings ChuckHastings approved these changes

@seunghwak seunghwak seunghwak approved these changes

Assignees

@naimnv naimnv

Labels
bug Something isn't working non-breaking Non-breaking change
Projects
None yet
Milestone
23.12
Development

Successfully merging this pull request may close these issues.

Several Possible Errors in Leiden Implementation Leiden Clustering produces too many clusters
5 participants
@naimnv @alexbarghi-nv @ChuckHastings @BradReesWork @seunghwak