-
Notifications
You must be signed in to change notification settings - Fork 313
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
[REVIEW] Expose resolution (gamma) parameter in Louvain #1034
[REVIEW] Expose resolution (gamma) parameter in Louvain #1034
Conversation
…here it will be used
Please update the changelog in order to start CI tests. View the gpuCI docs here. |
1) Reformulated the delta_Q computation, should actually match the change in modularity now 2) Refactored computing delta_Q into a separate function 3) Expose detail APIs for computing Q and delta_Q 4) Add unit test that specifically tests modularity and delta modularity functions for some simple graphs.
Codecov Report
@@ Coverage Diff @@
## branch-0.15 #1034 +/- ##
===============================================
+ Coverage 70.54% 70.57% +0.02%
===============================================
Files 54 54
Lines 1996 1998 +2
===============================================
+ Hits 1408 1410 +2
Misses 588 588
Continue to review full report at Codecov.
|
rerun tests |
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, one quick question inline which need not hold up my approval.
int max_iter = 100); | ||
vertex_t *louvain_parts, | ||
int max_iter = 100, | ||
weight_t resolution = weight_t{1}); |
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 assuming this default value is fine for ECG?
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.
The default of 1 makes Louvain behave just as it did before - which should be fine for ECG.
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.
👍
Add a resolution parameter (gamma in the mathematical formulation) to the Louvain score.
Closes #674
Includes an update to the delta_Q computation