Skip to content

perf: Combat perf improvements - #4070

Merged
flying-sheep merged 11 commits into
scverse:mainfrom
ilaykav:combat-perf-improvements
Apr 24, 2026
Merged

perf: Combat perf improvements#4070
flying-sheep merged 11 commits into
scverse:mainfrom
ilaykav:combat-perf-improvements

Conversation

@ilaykav

@ilaykav ilaykav commented Apr 17, 2026

Copy link
Copy Markdown
Contributor
  • Closes # - Perf Improvement
  • Tests included or not required because: all tests pass, Added combat benchmark
  • Release notes not necessary because: internal perf improvement

Devloped on venv, ran benchamrk locally, main changes:

  • Replaced np.dot(x, np.ones(...)) with numpy broadcasting ([:, np.newaxis])
  • Extract .values from DataFrames before computation loops
    Benchmarked locally on 50K cells × 500 genes, 2-3 speedup on pp.combat

@ilaykav ilaykav changed the title fix: Combat perf improvements perf: Combat perf improvements Apr 17, 2026
@ilaykav
ilaykav force-pushed the combat-perf-improvements branch from 7b601bf to c0e36aa Compare April 17, 2026 14:37
@flying-sheep flying-sheep added this to the 1.12.2 milestone Apr 17, 2026
@codecov

codecov Bot commented Apr 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.59%. Comparing base (d15aede) to head (f994269).
⚠️ Report is 68 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4070      +/-   ##
==========================================
- Coverage   78.60%   78.59%   -0.02%     
==========================================
  Files         118      118              
  Lines       12753    12745       -8     
==========================================
- Hits        10025    10017       -8     
  Misses       2728     2728              
Flag Coverage Δ
hatch-test.low-vers 77.89% <100.00%> (-0.02%) ⬇️
hatch-test.pre 78.47% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/scanpy/preprocessing/_combat.py 88.18% <100.00%> (-0.70%) ⬇️

@ilaykav

ilaykav commented Apr 17, 2026

Copy link
Copy Markdown
Contributor Author

~1.5x-2x times faster on 3K-20K cells, measured locally on my Intel i7-8665U python 3.14

@flying-sheep flying-sheep left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

hi, thanks! there are a few superficial issues.

About the general idea, why do you think that to_numpy() influences anything? For pandas arrays that already wrap underlying numpy arrays, the method should have basically no performance impact.

Comment thread benchmarks/benchmarks/tools.py Outdated
Comment thread benchmarks/benchmarks/tools.py Outdated
Comment thread src/scanpy/preprocessing/_combat.py Outdated
Comment thread src/scanpy/preprocessing/_combat.py Outdated
@scverse-benchmark

scverse-benchmark Bot commented Apr 23, 2026

Copy link
Copy Markdown

Benchmark changes

Change Before [d15aede] After [d94816f] Ratio Benchmark (Parameter)
- 495±30μs 389±3μs 0.79 preprocessing_counts.FastSuite.time_log1p('pbmc68k_reduced', 'counts-off-axis')
- 110±0.8ms 76.3±1ms 0.7 preprocessing_log.CombatSuite.time_combat

Comparison: https://github.com/scverse/scanpy/compare/d15aedeb0300d04bb327333e30bebfc0aa37d87c..d94816fc8f252ffa9a4f919aaacc80dbbf505eb4
Last changed: Fri, 24 Apr 2026 08:00:37 +0000

More details: https://github.com/scverse/scanpy/pull/4070/checks?check_run_id=72836821961

@ilaykav

ilaykav commented Apr 23, 2026

Copy link
Copy Markdown
Contributor Author

You're right, the broadcasting itself (and the memory save from removing the np.ones allocations, not the .to_numpy()) is what actually made the impact, dropping the .to_numpy() change

@ilaykav
ilaykav force-pushed the combat-perf-improvements branch from b15cf74 to d94816f Compare April 23, 2026 23:39
flying-sheep and others added 3 commits April 24, 2026 09:29
Co-authored-by: Copilot <copilot@github.com>
@flying-sheep
flying-sheep merged commit d6f2c51 into scverse:main Apr 24, 2026
14 checks passed
@flying-sheep

Copy link
Copy Markdown
Member

thank you!

flying-sheep pushed a commit that referenced this pull request Apr 24, 2026
…4085)

Co-authored-by: Ilay Kavitzky <ilay.kavitzky@gmail.com>
flying-sheep added a commit that referenced this pull request Apr 26, 2026
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Philipp A. <flying-sheep@web.de>
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.

2 participants