-
Notifications
You must be signed in to change notification settings - Fork 588
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
Use modern RNG #3041
base: main
Are you sure you want to change the base?
Use modern RNG #3041
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3041 +/- ##
==========================================
+ Coverage 75.87% 75.88% +0.01%
==========================================
Files 110 110
Lines 12536 12542 +6
==========================================
+ Hits 9512 9518 +6
Misses 3024 3024
|
Benchmark changes
Comparison: https://github.com/scverse/scanpy/compare/8d046ff37e024ae88eadfb22ea8fd142a6b95aa1..d9877c996b655a236f14fc242717a637365cd7d8 More details: https://github.com/scverse/scanpy/pull/3041/checks?check_run_id=25784166991 |
@RubenVanEsch could you look at this as well for your issue? I am not so sure if a 15% slowdown is acceptable given the fact that the answer to solving this could just be "use WSL," which we never got a response on because we go sidetracked. Could @RubenVanEsch or someone else confirm or not that the original problem is reproducible on WSL? |
Also @flying-sheep coming back to this - why doesn't this break tests? The underlying number generation mechanism is the same somehow? Or similar enough? |
@ilan-gold turns out i cant install WSL on my laptop after all, so unfortunately i cant check this. |
The other way round: If using WSL would be a viable workaround, we don’t need this. So if you can’t use WSL, it’s even more important that you help us by checking if this PR fixes things for you. |
Seems like this code is super performance sensitive: Having a Python implementation of
getrandbits
in 8572ecb resulted in a slowdown: