-
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
Refactor neighbors-based metrics to use NeighborsResults
#129
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #129 +/- ##
==========================================
- Coverage 93.77% 91.00% -2.77%
==========================================
Files 25 25
Lines 931 956 +25
==========================================
- Hits 873 870 -3
- Misses 58 86 +28
|
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.
LGTM!
Looked through and ran the tests.
Now for 500 000 cells the main time used is spent in pynndescent
(86%) and in fuzzy_simplical_set
(4%) see profiling results below. Also, tests are passing. test_kmeans
failed for me once locally but has been passing ever since.
Let me know if I should have a look at anything specific!
@jan-engelmann thanks! I will make a release soon |
Refactor neighbors-based metrics to use NeighborsResults, making it simpler for users as there is no confusion over passing distances or connectivities. Also reduces overhead of converting back and forth between sparse and dense representations of the neighbor results. Compute umap-based connectivities in this package instead of scanpy, and make the implementation more efficient Remove unnecessary vmap in pcr regression Update notebooks Drop usage of private scanpy functions. Scanpy can likely be dropped as a dependency in a future release if pca with implicit centering for sparse matrices makes it into sklearn Bump to 0.5.0 Fixes #109 as lisi always uses the dense (indices, distances) representation for neighbors. The issue arose when the approximate neighbors method gave a distance of zero to non-self cells. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Jan Engelmann <[email protected]>
NeighborsResults
, making it simpler for users as there is no confusion over passing distances or connectivities. Also reduces overhead of converting back and forth between sparse and dense representations of the neighbor results.Fixes #109 as lisi always uses the dense (indices, distances) representation for neighbors. The issue arose when the approximate neighbors method gave a distance of zero to non-self cells.