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.

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

Merged

Conversation

ChuckHastings
Copy link
Collaborator

Add a resolution parameter (gamma in the mathematical formulation) to the Louvain score.

Closes #674

Includes an update to the delta_Q computation

@ChuckHastings ChuckHastings requested review from a team as code owners August 5, 2020 14:59
@GPUtester
Copy link
Contributor

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.
@ChuckHastings ChuckHastings changed the title [WIP] Expose resolution (gamma) parameter in Louvain [REVIEW] Expose resolution (gamma) parameter in Louvain Aug 5, 2020
@codecov-commenter
Copy link

codecov-commenter commented Aug 5, 2020

Codecov Report

Merging #1034 into branch-0.15 will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@               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              
Impacted Files Coverage Δ
python/cugraph/community/louvain.py 88.88% <100.00%> (ø)
python/cugraph/_version.py 44.80% <0.00%> (+0.39%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb9b16f...e1e5d25. Read the comment docs.

@ChuckHastings
Copy link
Collaborator Author

rerun tests

Copy link
Contributor

@rlratzel rlratzel left a 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});
Copy link
Contributor

@rlratzel rlratzel Aug 5, 2020

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?

Copy link
Collaborator Author

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.

Copy link
Member

@afender afender left a comment

Choose a reason for hiding this comment

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

👍

@afender afender merged commit 18f5240 into rapidsai:branch-0.15 Aug 6, 2020
@ChuckHastings ChuckHastings deleted the fea_expose_louvain_gamma branch February 10, 2021 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] Expose gamma (resolution) parameter for Louvain.
6 participants