-
Notifications
You must be signed in to change notification settings - Fork 12
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
leiden nmi ari #24
leiden nmi ari #24
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #24 +/- ##
==========================================
- Coverage 93.75% 93.23% -0.52%
==========================================
Files 9 9
Lines 288 325 +37
==========================================
+ Hits 270 303 +33
- Misses 18 22 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
||
def test_nmi_ari_cluster_labels_leiden_parallel(): | ||
X, labels = dummy_x_labels(return_symmetric_positive=True) | ||
nmi, ari = scib_metrics.nmi_ari_cluster_labels_leiden(X, labels, optimize_resolution=True, n_jobs=2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible to check this against some leiden impl to make sure the clusters are the same? Is this reliable for diff random seeds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah jk i see its leiden vs louvain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leiden is faster. And the igraph implementation is even faster than the one scanpy uses
src/scib_metrics/_ari_nmi.py
Outdated
) | ||
except ImportError: | ||
logger.info("Using for loop over resolutions. pip install joblib for parallelization.") | ||
out = [nmi_ari_cluster_labels_leiden(X, labels, False, r) for r in resolutions] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this recursive pattern feels like a recipe for bugs. would be much cleaner to just split out the else case into a helper then call that from both cases (optimize or not optimize)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
speedy
Adds leiden nmi ari scores to more closely match scib (they use louvain). This does a search of 10 resolutions of leiden clustering to pick the optimal NMI (as in scIB, but it uses 20 res params)