Skip to content

Conversation

@selmanozleyen
Copy link
Member

@selmanozleyen selmanozleyen commented Sep 9, 2025

Solves #431

initial measurements

gpu w kernels
Time taken: 0.18562102317810059 seconds
old cpu
Time taken: 26.695605278015137 seconds

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@selmanozleyen selmanozleyen requested a review from Intron7 October 16, 2025 09:33
@selmanozleyen
Copy link
Member Author

@Intron7 if this looks good to you then I will remove the tmp_scripts folder. But so far its ready for merge

@Intron7 Intron7 requested a review from Zethson January 21, 2026 16:47
@Intron7 Intron7 added run-gpu-ci runs GPU CI and removed run-gpu-ci runs GPU CI labels Jan 21, 2026
@Intron7 Intron7 added the run-gpu-ci runs GPU CI label Jan 22, 2026
@Zethson Zethson changed the title E-Distance by scverse pertpy into rapids_singlecell Add pertpy's E-Distance Jan 23, 2026
Copy link
Member

@Zethson Zethson left a 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!

  1. 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.
  2. @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.
  3. @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.
  4. @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! ❤️

@Intron7 Intron7 requested a review from Zethson January 23, 2026 15:08
@Intron7 Intron7 added run-gpu-ci runs GPU CI and removed run-gpu-ci runs GPU CI labels Jan 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-gpu-ci runs GPU CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants