-
Notifications
You must be signed in to change notification settings - Fork 20
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
Added approximate KNN backend #791
base: main
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
@Zethson what is your stance on importing private variables from scanpy, as we are doing elsewhere too? I remember in the past you've added a fix of a previously imported, external private variable by defining the required variable ourselves. I lean towards doing this here too |
transformer | ||
Approximate kNN search implementation following the API of | ||
:class:`~sklearn.neighbors.KNeighborsTransformer`. | ||
See :doc:`/how-to/knn-transformers` for more details. |
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.
The link breaks in our documentation as this was from the scanpy doc; but we could insert the full scanpy doc link instead https://scanpy.readthedocs.io/en/latest/how-to/knn-transformers.html & make a quick comment that this is scanpy but works the same for ehrapy
`'pynndescent'` | ||
:class:`~pynndescent.pynndescent_.PyNNDescentTransformer` | ||
`'rapids'` | ||
A transformer based on :class:`cuml.neighbors.NearestNeighbors`. |
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.
Here can either add "deprecated since scanpy version 1.10.0" such that its clear where 1.10.0 comes from, or hide this from the beginning as its deprecated already?
Then, would also need to remove from _KnownTransformer
if we write this ourselves see question on what to do about private variables below
Yeah, we should try to avoid importing internals so I think that's a good option. |
Thank you so much @nicolassidoux ! But this is from our side so far a lacking suite of tests (as we as of now have been closely eyeing scanpy), and hence no test suite at this point to add to here :) |
Co-authored-by: Eljas Roellin <[email protected]>
ready for me to take a quick look again or are you still doing stuff? :) |
I had to
from scanpy.neighbors._types import KnnTransformerLike, _KnownTransformer
to make transformer argument ofneighbors()
consistent with scanpy. But _types is protected...