Skip to content

Sum aggregate doesn't include nan values#7009

Merged
gatesn merged 3 commits intodevelopfrom
rk/nonnansum
Mar 19, 2026
Merged

Sum aggregate doesn't include nan values#7009
gatesn merged 3 commits intodevelopfrom
rk/nonnansum

Conversation

@robert3005
Copy link
Contributor

@robert3005 robert3005 commented Mar 18, 2026

NaN will cause sum to be NaN. Instead we have NaNCount already that users can
use to handle columns with NaNs.

fix #5152

Signed-off-by: Robert Kruszewski github@robertk.io

Signed-off-by: Robert Kruszewski <github@robertk.io>
@robert3005 robert3005 added the changelog/fix A bug fix label Mar 18, 2026
@gatesn
Copy link
Contributor

gatesn commented Mar 18, 2026

You should also make the is_saturated check test for nans since +Inf + -Inf = NaN, in which case you're saturating anyways

robert3005 and others added 2 commits March 18, 2026 12:06
Signed-off-by: Robert Kruszewski <github@robertk.io>
Signed-off-by: Nicholas Gates <nick@nickgates.com>
@gatesn gatesn enabled auto-merge (squash) March 19, 2026 00:46
@codspeed-hq
Copy link

codspeed-hq bot commented Mar 19, 2026

Merging this PR will degrade performance by 15.51%

⚡ 1 improved benchmark
❌ 2 regressed benchmarks
✅ 1006 untouched benchmarks
⏩ 1515 skipped benchmarks1

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation take_map[(0.1, 1.0)] 4 ms 3.6 ms +11.53%
Simulation bitwise_not_vortex_buffer_mut[1024] 477.2 ns 535.6 ns -10.89%
Simulation bitwise_not_vortex_buffer_mut[128] 317.8 ns 376.1 ns -15.51%

Comparing rk/nonnansum (d3d312c) with develop (644fcdf)

Open in CodSpeed

Footnotes

  1. 1515 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@gatesn gatesn merged commit 5c80b40 into develop Mar 19, 2026
53 of 54 checks passed
@gatesn gatesn deleted the rk/nonnansum branch March 19, 2026 00:52
dimitarvdimitrov pushed a commit that referenced this pull request Mar 20, 2026
NaN will cause sum to be NaN. Instead we have NaNCount already that
users can
use to handle columns with NaNs.

fix #5152 

Signed-off-by: Robert Kruszewski <github@robertk.io>

---------

Signed-off-by: Robert Kruszewski <github@robertk.io>
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Co-authored-by: Nicholas Gates <nick@nickgates.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/fix A bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sum statistic should not include NaNs

3 participants