feat: add modularity to scanpy.metrics#3613
Conversation
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
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)
|
Oh, I just saw that we have |
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.
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
This reverts commit 19cecaa.
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: