Added approximate KNN backend #791
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.
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.
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, _KnownTransformerto make transformer argument ofneighbors()consistent with scanpy. But _types is protected...