feat: add modularity to scanpy.metrics #3613
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
flying-sheep
requested changes
Apr 25, 2025
Member
There was a problem hiding this comment.
Hi! Apart from the issue with igraph being an optional dependency, this looks good!
I think we have get_igraph_from_adjacency which might be useful, but maybe not.
Could you please add tests? We have many examples on how, best would probably be
- for the direct variant, manually create very small graphs to run this on so you can be sure the results are correct
- for the anndata version, use
neighborsto create the connectivity matrix.
Please add @needs.igraph so the test only runs when igraph is installed
if you’re unsure about anything, please search the code for examples or ask me!
If you end up implementing a non-igraph flavor for this, please test using parametrization, e.g.: @pytest.mark.parametrize("directed", [True, False], ids=["directed", "undirected"])
… on how to integrate the two
flying-sheep
requested changes
May 8, 2025
Member
There was a problem hiding this comment.
Hi, this is shaping up! The tests are looking particularly great!
Please address these previous comments:
- Please follow the doc style as described here: #3613 (comment)
Also I mentioned in there “But here a is_directed: bool parameter would be better anyway, there will never be more than two options.”
What do you think?
- Don’t densify (in
modularity_adataor anywhere): #3613 (comment) - Why the
np.asarray? #3613 (comment)
Member
|
Oh, I just saw that we have |
flying-sheep
requested changes
May 16, 2025
Member
There was a problem hiding this comment.
Looks good! A few points:
- please add a release note (see the other PR, please adapt the PR number in the command I mention there when running it for this PR)
- please move the correct change to
unique_labels(to_numpy) from the other PR into this one:scanpy/src/scanpy/metrics/_metrics.py
Line 63 in 040b8b7
- please change
modetois_directed: bool(no default value) formodularity
Once your other PR (the neighbors one) is merged, let’s finish this one up:
- we should make
neighborsstore anis_directedparam in.uns[key_added or "neighbors"]["params"] - then we change the
modularity_adataparameterobsp: strparameter tokey: str = "neighbors" - then we use
adata.uns[key]["connectivities_key"]andadata.uns[key]["params"]["is_directed"]inmodularity_adatato callmodularity.
flying-sheep
changed the title
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
flying-sheep
changed the title
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds two functions to compute modularity scores from a given graph and a clustering like Leiden or Louvain. The goal is to make it easier to compare different community detection methods using an external metric. To my knowledge, there is no built-in way to compare clustering results nor ways to calculate modularity score.
Fixes #2908
Functions: