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

Fix initial modularity for walktrap #1974

Merged
merged 2 commits into from Feb 25, 2022
Merged

Fix initial modularity for walktrap #1974

merged 2 commits into from Feb 25, 2022

Conversation

vtraag
Copy link
Member

@vtraag vtraag commented Feb 23, 2022

Walktrap maintains a list of modularity along each step of its merging of various communities. The initial modularity was always incorrectly set to 0, instead of the modularity of the singleton partition. Walktrap returns the actual membership that contains the maximum modularity.

The actual partition that was identified in walktrap in #1927 is the partition that puts the whole graph into a single cluster, which has a modularity of 0. However, because the initial modularity before any merge was incorrectly set to 0, it incorrectly returned the singleton partition instead. This PR fixes #1927.

@vtraag vtraag requested a review from szhorvat February 23, 2022 15:09
@codecov
Copy link

codecov bot commented Feb 23, 2022

Codecov Report

Merging #1974 (1f0ab51) into develop (2620107) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1974   +/-   ##
========================================
  Coverage    74.94%   74.94%           
========================================
  Files          347      347           
  Lines        57893    57898    +5     
========================================
+ Hits         43387    43394    +7     
+ Misses       14506    14504    -2     
Impacted Files Coverage Δ
src/community/walktrap/walktrap_communities.cpp 62.08% <100.00%> (+0.35%) ⬆️
src/community/walktrap/walktrap.cpp 91.30% <0.00%> (+8.69%) ⬆️

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 2620107...1f0ab51. Read the comment docs.

@szhorvat
Copy link
Member

My only comment is that it would be nice to backport this to 0.9 as well so it can go into the 1.3 R release... but if you think it's not important, just skip it.

@vtraag
Copy link
Member Author

vtraag commented Feb 23, 2022

Yeah, I agree, I will backport to 0.9.

@vtraag vtraag merged commit 2cefbcf into igraph:develop Feb 25, 2022
@vtraag vtraag deleted the fix/1927 branch February 25, 2022 16:06
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.

None yet

2 participants