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

Greedy modularity optimization community detection algorithm #314

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

olegfafurin
Copy link

Greedy modularity optimization algorithm as per paper of Newman

Copy link

codecov bot commented Nov 15, 2023

Codecov Report

Attention: 70 lines in your changes are missing coverage. Please review.

Comparison is base (cc3052f) 97.26% compared to head (bb85ea6) 96.72%.
Report is 3 commits behind head on master.

❗ Current head bb85ea6 differs from pull request most recent head ac77ee5. Consider uploading reports for the commit ac77ee5 to get more accurate results

Files Patch % Lines
src/community/greedy_modularity.jl 0.00% 70 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #314      +/-   ##
==========================================
- Coverage   97.26%   96.72%   -0.54%     
==========================================
  Files         115      113       -2     
  Lines        6795     6201     -594     
==========================================
- Hits         6609     5998     -611     
- Misses        186      203      +17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@gdalle gdalle left a comment

Choose a reason for hiding this comment

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

Good first implem!

src/community/greedy_modularity.jl Outdated Show resolved Hide resolved
src/community/greedy_modularity.jl Outdated Show resolved Hide resolved
src/community/greedy_modularity.jl Outdated Show resolved Hide resolved
src/community/greedy_modularity.jl Outdated Show resolved Hide resolved
src/community/greedy_modularity.jl Outdated Show resolved Hide resolved
src/community/greedy_modularity.jl Outdated Show resolved Hide resolved
src/community/greedy_modularity.jl Outdated Show resolved Hide resolved
src/community/greedy_modularity.jl Outdated Show resolved Hide resolved
src/community/greedy_modularity.jl Outdated Show resolved Hide resolved
test/community/greedy_modularity.jl Outdated Show resolved Hide resolved
@gdalle
Copy link
Member

gdalle commented Nov 21, 2023

Try a weighted version and for every number that is not an integer, use the eltype of the weights matrix to initialize it

src/community/greedy_modularity.jl Outdated Show resolved Hide resolved
src/community/greedy_modularity.jl Outdated Show resolved Hide resolved
src/community/greedy_modularity.jl Outdated Show resolved Hide resolved
test/community/greedy_modularity.jl Show resolved Hide resolved
m = sum(w[src(e), dst(e)] for e in edges(g)) * 2
n_groups = maximum(c)
a = zeros(modularity_type, n_groups)
e = zeros(modularity_type, n_groups, n_groups)
Copy link
Member

Choose a reason for hiding this comment

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

Could we try a dict or sparse matrix here?

return rewrite_class_ids(cs[imax])
end

function modularity_greedy_step!(
Copy link
Member

Choose a reason for hiding this comment

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

Test that this loop does not allocate

n = nv(g)
dq_max::typeof(Q) = typemin(Q)
to_merge::Tuple{Int,Int} = (0, 0)
for edge in edges(g)
Copy link
Member

Choose a reason for hiding this comment

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

investigate the case of self-loops

Copy link
Author

Choose a reason for hiding this comment

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

Self-loops indeed appear in modularity computation (in a correct way)
They do participate in modularity optimization step and they can also impact which merge is optimal. Yet we never merge a cluster with itself as we check that ends of an edge belong to different clusters at traversal stage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge Do not merge this PR (yet)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants