Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use modern RNG #3041

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Use modern RNG #3041

wants to merge 5 commits into from

Conversation

flying-sheep
Copy link
Member

@flying-sheep flying-sheep commented Apr 30, 2024 •
edited

  • Release notes not necessary because:

Seems like this code is super performance sensitive: Having a Python implementation of getrandbits in 8572ecb resulted in a slowdown:

Change Before [0d4554b] After [1b2d9dd] Ratio Benchmark (Parameter)
+ 15.2±0.03ms 31.7±0.1ms 2.09 preprocessing.time_highly_variable_genes

@flying-sheep flying-sheep mentioned this pull request Apr 30, 2024
Closed
3 tasks
@codecov Codecov
Copy link

codecov bot commented Apr 30, 2024 •
edited

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.88%. Comparing base (8d046ff) to head (d9877c9).
Report is 2 commits behind head on main.

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
Files Coverage Δ
scanpy/_utils/__init__.py 75.41% <100.00%> (+0.31%) ⬆️

@scverse-benchmark scverse-benchmark
Copy link

scverse-benchmark bot commented Apr 30, 2024 •
edited

Benchmark changes

Change Before [8d046ff] After [d9877c9] Ratio Benchmark (Parameter)
+ 15.1±0.05ms 17.2±0.1ms 1.14 tools.time_leiden

Comparison: https://github.com/scverse/scanpy/compare/8d046ff37e024ae88eadfb22ea8fd142a6b95aa1..d9877c996b655a236f14fc242717a637365cd7d8
Last changed: Tue, 4 Jun 2024 12:01:33 +0000

More details: https://github.com/scverse/scanpy/pull/3041/checks?check_run_id=25784166991

@flying-sheep flying-sheep added this to the 1.10.2 milestone Apr 30, 2024
@ilan-gold
Copy link
Contributor

@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?

@flying-sheep flying-sheep self-assigned this May 2, 2024
@ilan-gold
Copy link
Contributor

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?

@RubenVanEsch
Copy link

@ilan-gold turns out i cant install WSL on my laptop after all, so unfortunately i cant check this.

@flying-sheep
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ilan-gold ilan-gold Awaiting requested review from ilan-gold

Assignees

@flying-sheep flying-sheep

Labels
benchmark
Projects
None yet
Milestone
1.10.2
Development

Successfully merging this pull request may close these issues.

leiden alg with igraph flavor causes out of bounds freezing
3 participants
@flying-sheep @ilan-gold @RubenVanEsch