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

Several Possible Errors in Leiden Implementation #3850

Closed
fantaosha opened this issue Sep 7, 2023 · 1 comment · Fixed by #3990
Closed

Several Possible Errors in Leiden Implementation #3850

fantaosha opened this issue Sep 7, 2023 · 1 comment · Fixed by #3990
Assignees
Labels
bug Something isn't working

Comments

@fantaosha
Copy link

fantaosha commented Sep 7, 2023

Thanks for your great work! However, after reading the code of Leiden, I'm afraid that there are probably several errors in refine_impl.cuh.

  1. In the computation of well-connected vertices, I think the correct implementation should be
return wcut > (resolution * wdeg * (louvain_volume - wdeg)) / total_edge_weight;

According to the paper, Eq. (A4) rather than Eq. (A5) is used in the algorithms and proofs. Note that Eq. (A4) is CPM while Eq. (A5) is modularity. Therefore, if modularity is used, there should be a division of total_edge_weight. This also applies to the computation of well connected communities, which should be

bool is_dst_leiden_cluster_well_connected =
      dst_leiden_cut_to_louvain > resolution * dst_leiden_volume * 
             (louvain_cluster_volume - dst_leiden_volume) / total_edge_weight;
  1. I think the computation of modularity gain should be
mod_gain = aggregated_weight_to_neighboring_leiden_cluster -
                   resolution * src_weighted_deg * dst_leiden_volume / total_edge_weight;

since the vertex has not been merged into dst_leiden_cluster.

@fantaosha fantaosha changed the title Probably Several Errors in Leiden Implementation Several Possible Errors in Leiden Implementation Sep 7, 2023
@ChuckHastings ChuckHastings added the bug Something isn't working label Sep 26, 2023
@naimnv
Copy link
Contributor

naimnv commented Nov 8, 2023

Thank you. It somehow slipped my mind for a while. After quite some debugging we fixed those errors.

rapids-bot bot pushed a commit that referenced this issue Nov 20, 2023
- 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

Authors:
  - Naim (https://github.com/naimnv)
  - Alex Barghi (https://github.com/alexbarghi-nv)

Approvers:
  - Chuck Hastings (https://github.com/ChuckHastings)
  - Seunghwa Kang (https://github.com/seunghwak)
  - Brad Rees (https://github.com/BradReesWork)

URL: #3990
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants