Skip to content

(perf): remove out of core median in normalize_total - #3379

Merged
flying-sheep merged 8 commits into
mainfrom
remove-out-of-core-median
Apr 14, 2025
Merged

(perf): remove out of core median in normalize_total#3379
flying-sheep merged 8 commits into
mainfrom
remove-out-of-core-median

Conversation

@Intron7

@Intron7 Intron7 commented Nov 21, 2024

Copy link
Copy Markdown
Member

Computing counts per cell for normalize_total leads to better performance.

@Intron7 Intron7 added this to the 1.11.0 milestone Nov 21, 2024
@codecov

codecov Bot commented Nov 21, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.51%. Comparing base (f0f6960) to head (e2888c0).
Report is 40 commits behind head on main.

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3379   +/-   ##
=======================================
  Coverage   75.50%   75.51%           
=======================================
  Files         113      113           
  Lines       13140    13141    +1     
=======================================
+ Hits         9922     9924    +2     
+ Misses       3218     3217    -1     
Files with missing lines Coverage Δ
src/scanpy/preprocessing/_normalization.py 82.02% <100.00%> (+1.34%) ⬆️

@flying-sheep

Copy link
Copy Markdown
Member

Fair enough. Can this blow up? I.e. is counts ever big enough that this is a problem?

@flying-sheep flying-sheep modified the milestones: 1.11.0, 1.12.0 Dec 20, 2024
@Intron7

Intron7 commented Feb 10, 2025

Copy link
Copy Markdown
Member Author

So I did some testing with 1Billion cells. 1Billion cells at 64bit are roughly 8GB. The median costs another 8GB. Thats roughly 16GB. I dont think 16GB of Ram is an issue for someone that analyses 1Billion cells.

@Intron7
Intron7 requested a review from ilan-gold February 10, 2025 15:36
@ilan-gold

Copy link
Copy Markdown
Contributor

Why is this for the 1.12 milestone?

@flying-sheep flying-sheep changed the title remove out of core median (perf): remove out of core median in normalize_total Feb 18, 2025
@flying-sheep flying-sheep modified the milestones: 1.12.0, 1.11.1 Feb 18, 2025
@flying-sheep

Copy link
Copy Markdown
Member

probably because no 1.11.1 milestone existed for a bit (my fault, I’ll be cleaner about this in the future)

@ilan-gold ilan-gold left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test to get the coverage to pass, but the code itself looks fine.

@flying-sheep flying-sheep modified the milestones: 1.11.1, 1.11.2 Mar 31, 2025
@ilan-gold ilan-gold assigned ilan-gold and selmanozleyen and unassigned Intron7 Apr 3, 2025
@flying-sheep
flying-sheep enabled auto-merge (squash) April 14, 2025 12:23
@flying-sheep
flying-sheep disabled auto-merge April 14, 2025 12:42
@flying-sheep

Copy link
Copy Markdown
Member

Looks like the tests don’t pass anymore. Any idea what’s wrong?

@selmanozleyen

Copy link
Copy Markdown
Member

They passed the same tests on the PR right? The results I checked were due to small differences I am not sure why

@flying-sheep

Copy link
Copy Markdown
Member

Ah, it’s unrelated. The test that failed doesn’t call normalize_total.

@flying-sheep
flying-sheep enabled auto-merge (squash) April 14, 2025 13:00
@flying-sheep
flying-sheep merged commit 7f3f89a into main Apr 14, 2025
@flying-sheep
flying-sheep deleted the remove-out-of-core-median branch April 14, 2025 13:07
meeseeksmachine pushed a commit to meeseeksmachine/scanpy that referenced this pull request Apr 14, 2025
selmanozleyen added a commit to selmanozleyen/scanpy that referenced this pull request Apr 14, 2025
Co-authored-by: Philipp A. <flying-sheep@web.de>
Co-authored-by: Selman Özleyen <32667648+selmanozleyen@users.noreply.github.com>
flying-sheep pushed a commit that referenced this pull request Apr 14, 2025
…al (#3588)

Co-authored-by: Severin Dicks <37635888+Intron7@users.noreply.github.com>
flying-sheep added a commit that referenced this pull request Apr 25, 2025
Co-authored-by: Philipp A. <flying-sheep@web.de>
Co-authored-by: Selman Özleyen <32667648+selmanozleyen@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants