Return null for mean of all-null input#8389
Conversation
Merging this PR will improve performance by 19.61%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | rebuild_naive |
109.1 µs | 91.2 µs | +19.61% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing mitko/aggregate/mean-all-null-returns-null (445ff1c) with develop (49f300d)
Footnotes
-
4 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. ↩
| // the division produces 0/0 = NaN. The mean of an empty group is null (as in SQL), so | ||
| // mask out zero-count entries. This matches `finalize_scalar`. | ||
| let non_empty = count.binary( | ||
| ConstantArray::new(0u64, count.len()).into_array(), |
There was a problem hiding this comment.
doesn't this need to be target dtype?
|
sorry this was prematurely opened by claude. i'm not sure about the |
| (vec![Some(f64::NAN), Some(1.0), None], Some(0.5)), | ||
| (vec![Some(f64::NAN), Some(1.0), Some(1.0)], Some(2.0 / 3.0)), | ||
| (vec![None, None, Some(f64::NAN)], Some(0.0)), | ||
| (vec![Some(f64::NAN), Some(1.0), Some(1.0)], Some(2.0 / 3.0)), |
|
I am going to drop this here https://pandas.pydata.org/docs/reference/api/pandas.DataFrame.mean.html. I think long term we will need |
i just found out about #7009, i think we'll need the option now so we have the default stats adhere to the IEEE standard and our stats be more useful. WIP |
|
we talked about changing only stats to contain the sum as "sum of non-NaNs." But in order to use the sum stat, we have to now always look for the NanCount stat and compute it if it's not present. are we ok with that perf change for old arrays? I suppose we could do the sum and the nan check in one pass and not change it too much, but that'd be a new accumulator implementation; do you guys have better ideas? |
|
We need to compute it anyway to make this correct. I am working through adding skipNaN option to our aggregates |
|
This PR has been marked as stale because it has been open for 14 days with no activity. Please comment or remove the stale label if you wish to keep it active, otherwise it will be closed in 7 days |
Mean::finalize (the grouped array path) divided the sum and count arrays directly, so an all-null group produced 0/0 = NaN, while finalize_scalar returned s/c for the same input and also produced NaN. Nulls are skipped during accumulation, so a zero-count group is an empty mean and should be null, as in SQL. Mask out zero-count entries after the division in finalize, and add a zero-count arm to finalize_scalar, so both paths return null. NaN handling is unchanged: NumericalAggregateOpts::skip_nans (the default, from #8457) still controls whether NaN values poison the result. The new tests run the same groups (NaN/null mixtures, all-null, all-valid) through both the scalar-partial path and the grouped array path with identical assertions. Signed-off-by: Dimitar Dimitrov <dimitar@spiraldb.com>
dimitarvdimitrov
force-pushed
the
mitko/aggregate/mean-all-null-returns-null
branch
from
561876e to
445ff1c
Compare
3 participants
Reverts #8365: a mean over zero non-null values should be null (as in SQL, where nulls are skipped and an empty mean is null), not NaN.
Both reduce paths previously produced
0/0 = NaNfor an all-null group:Mean::finalizedivided the sum and count arrays directly, andMean::finalize_scalarcomputedsum / count. Nowfinalizemasks out zero-count entries andfinalize_scalarreturns null for a zero count, so both paths return null.NaN handling is unchanged; it stays controlled by
NumericalAggregateOpts::skip_nansfrom #8457.