-
Notifications
You must be signed in to change notification settings - Fork 40
Add pertpy's E-Distance #454
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
base: main
Are you sure you want to change the base?
Add pertpy's E-Distance #454
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
@Intron7 if this looks good to you then I will remove the |
Zethson
left a comment
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.
Thank both of you very much! This is super cool. I'm still learning and you're the experts so my comments aren't the most helpful (yet).
Generally, I find the code pretty readable and nice. Great!
- Has RSC adopted typehints in docstrings? I would remove these again and just use the inherent Python typing. There's no advantage to typehints in docstrings because IDEs recognize them from the code. It can be annoying to have to keep them in sync.
- @selmanozleyen could you please write a proper PR description where you outline what you're adding and have a small copy and pastable example? Ideally, users that come across this PR should immediately learn what new feature this is and how they can use it.
- @selmanozleyen instead of always passing a numerical random for the random state, we can generalize this to pass an int or RNG object or whatever. This is also well done in scanpy and I suggest that we stick to how it's done there.
- @Intron7 this is more of a learning question from me but we're often hardcoding float32 for the cupy matrices and I wonder whether we should use the existing data type (which can e.g. be float64) that the non cupy matrices had instead?
For now, I generally just checked the overall code design and correctness. If I find the time in the next 1-2 weeks, I might also check the amazing kernels of Severin although you're the experts on that of course.
Feel free to merge it without a second check of mine of course.
Thanks! ❤️
Solves #431
initial measurements