Skip to content

perf: welford's algorithm for mean-var aggregation#4147

Merged
ilan-gold merged 19 commits into
mainfrom
ig/welford
Jun 23, 2026
Merged

perf: welford's algorithm for mean-var aggregation#4147
ilan-gold merged 19 commits into
mainfrom
ig/welford

Conversation

@ilan-gold

Copy link
Copy Markdown
Contributor

From discussions with @zboldyga.

This should then in theory be reused with #4143 instead of its custom moments calculation

  • Closes #
  • Tests included or not required because:

@ilan-gold ilan-gold added this to the 1.12.2 milestone Jun 8, 2026
@ilan-gold ilan-gold changed the title perf: welford's algorithm for mean-var perf: welford's algorithm for mean-var aggregation Jun 8, 2026
out[cat, col] += data.data[j]


@njit

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We should make these nogil or provide an option for fau to provide nogil njit

@codecov

codecov Bot commented Jun 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.65%. Comparing base (62d4c97) to head (ff0ac25).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4147      +/-   ##
==========================================
- Coverage   79.68%   79.65%   -0.04%     
==========================================
  Files         120      120              
  Lines       12801    12795       -6     
==========================================
- Hits        10201    10192       -9     
- Misses       2600     2603       +3     
Flag Coverage Δ
hatch-test.low-vers 78.90% <100.00%> (-0.04%) ⬇️
hatch-test.pre 79.52% <100.00%> (-0.03%) ⬇️

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

Files with missing lines Coverage Δ
src/scanpy/get/_aggregated.py 92.66% <100.00%> (-0.65%) ⬇️
src/scanpy/get/_kernels.py 100.00% <ø> (ø)

... and 1 file with indirect coverage changes

@ilan-gold ilan-gold requested a review from flying-sheep June 8, 2026 12:24

@zboldyga zboldyga 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.

@ilan-gold I reviewed since this plays into our other work

I don't see any issues with the welfords implementation, lgtm!

and overall I'm new with njit, OMP, TBB. But I see your point about nogil, and was able to create some scenarios that trigger that path. So agreed on the suggestion... I also found that it does also slightly improve the chan njit work when fau falls back on the serial build, and I guess it would generally fix any of these types of things across scanpy, so maybe that does belong in fau?

There's probably some threading stuff I'm not fully grasping with scanpy, fau, those libraries yet though; I will be more confident in that in a few weeks. I figure I will revisit threading and numba as a whole as part of building a good understanding of these libraries, and if I find any thread issues throughout scanpy/fau I will raise them at that point.

@ilan-gold

Copy link
Copy Markdown
Contributor Author

so maybe that does belong in fau?

Yes the issue with nogil is that it means your code is no longer threadsafe with respect to its inputs (in a certain mental model of things, I guess). So introducing this change needs some though - I don't think anything in FAU actually alters its inputs though so we should be good.

There's probably some threading stuff I'm not fully grasping with scanpy, fau, those libraries yet though; I will be more confident in that in a few weeks. I figure I will revisit threading and numba as a whole as part of building a good understanding of these libraries, and if I find any thread issues throughout scanpy/fau I will raise them at that point.

That would be amazing because it is something we have (clearly) struggled with

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

nice! some questions.

Comment thread docs/release-notes/4147.perf.md
Comment thread src/scanpy/get/_aggregated.py Outdated
Comment thread src/scanpy/get/_kernels.py
@scverse-benchmark

scverse-benchmark Bot commented Jun 18, 2026

Copy link
Copy Markdown

Benchmark changes

Change Before [39f1241] After [25e6bfc] Ratio Benchmark (Parameter)
+ 131±1ms 182±2ms 1.39 preprocessing_counts.Agg.time_agg('var', False)
+ 49.0±2ms 70.2±4ms 1.43 preprocessing_counts.Agg.time_agg('var', True)

Comparison: https://github.com/scverse/scanpy/compare/39f12414fea9cea9439a3d9f665d1e17636092a9..25e6bfcdbb14be0ca79a555e0939fe81dfe34bdb
Last changed: Mon, 22 Jun 2026 12:58:40 +0000

More details: https://github.com/scverse/scanpy/pull/4147/checks?check_run_id=82708559802

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

Was bd85e03 supposed to be a response to the benchmark results? because it didn’t change them.

Looks great, but maybe the test can be made slightly more clear.

Comment thread tests/test_aggregated.py Outdated
Comment thread tests/test_aggregated.py Outdated
Comment thread tests/test_aggregated.py
@ilan-gold

Copy link
Copy Markdown
Contributor Author

Was bd85e03 supposed to be a response to the benchmark results? because it didn’t change them.

It was certainly an attempt! The number did go down a bit, but not much

@ilan-gold ilan-gold enabled auto-merge (squash) June 22, 2026 11:35
@ilan-gold ilan-gold merged commit 6afdc6e into main Jun 23, 2026
14 checks passed
@ilan-gold ilan-gold deleted the ig/welford branch June 23, 2026 13:08
ilan-gold added a commit that referenced this pull request Jun 23, 2026
…n-var aggregation) (#4176)

Co-authored-by: Ilan Gold <ilanbassgold@gmail.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.

3 participants