search

theislab / ehrapy

Electronic Health Record Analysis with Python.
https://ehrapy.readthedocs.io/
Apache License 2.0
237 stars 19 forks source link

Added approximate KNN backend #791

Closed nicolassidoux closed 2 weeks ago

nicolassidoux commented 3 months ago

I had to from scanpy.neighbors._types import KnnTransformerLike, _KnownTransformer to make transformer argument of neighbors() consistent with scanpy. But _types is protected...

eroell commented 3 months ago

@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

Zethson commented 3 months ago

@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

Yeah, we should try to avoid importing internals so I think that's a good option.

eroell commented 3 months ago

Thank you so much @nicolassidoux ! One thing I realized here was that we should add tests for the pass through function calls of scanpy...

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 :)

eroell commented 1 month ago

ready for me to take a quick look again or are you still doing stuff? :)

nicolassidoux commented 2 weeks ago

Hi @eroell, ready for re-review!

eroell commented 2 weeks ago

Added the intersphinx mappings for pynndescent, and linked the reference to the scanpy knn tutorial with a readable text; Also added a more wordy description on the arguments, more wordy than scanpy... So direct commit of suggestions instead of knitty-gritty review comments to save your time, what is your take on these additions @nicolassidoux? Would you as user think this is more clear now, or worse?

nicolassidoux commented 2 weeks ago

@eroell Yes, direct commit is a good idea indeed! In regard with documentation stuff, I really let you decide whatever suits, I definitely don't have the skills to judge from a user perspective :smile: But AFAIK the description looks good! I let you close the PR if you think all is good!

eroell commented 2 weeks ago

great - this is a nice addition, thank you @nicolassidoux !