fix(rust, python) Summation on empty series evaluates to Some(0)
#5773
Conversation
andrewpollack
force-pushed
the
empty-series-sum-zero
branch
from
2c235f8 to
4ee4c69
Compare
polars/polars-core/src/series/mod.rs
Outdated
| @@ -1038,6 +1047,23 @@ mod test { | |||
| let _ = series.slice(4, 2); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn empty_series_sum_evals_zero() { | |||
There was a problem hiding this comment.
Could these tests moved to the python tests? We want to reduce the testing ci time. Rust tests need a long time to compile/run.
There was a problem hiding this comment.
Sure! Looking to move now, and resolving why the Python test for an empty sum is still returning None
andrewpollack
force-pushed
the
empty-series-sum-zero
branch
3 times, most recently
from
31d701e to
1b1963c
Compare
|
From https://github.com/pola-rs/polars/blob/master/py-polars/src/series.rs#L482 , it looks like I'll need to adjust |
andrewpollack
force-pushed
the
empty-series-sum-zero
branch
from
1b1963c to
5d9302e
Compare
| /// | ||
| /// If the [`DataType`] is one of `{Int8, UInt8, Int16, UInt16}` the `Series` is | ||
| /// first cast to `Int64` to prevent overflow issues. | ||
| pub fn sum_as_series(&self) -> Series { | ||
| use DataType::*; | ||
| if self.is_empty() && self.dtype().is_numeric() { | ||
| return Series::new("", [0]) |
There was a problem hiding this comment.
Almost there. Int8 | UInt8 | Int16 | UInt16 should be Int64.
There was a problem hiding this comment.
Since we recursively call into sum_as_series, won't they be converted to Int64 on the second call? Or should I implement this logic to save on an unnecessary call?
There was a problem hiding this comment.
Oh, right. Missed that one.
Nope. This is good!
There was a problem hiding this comment.
Awesome! Thank you for your feedbacks on this PR 😸
andrewpollack
changed the title
Some(0)Some(0)
github-actions
bot
added
fix
|
Thanks @andrewpollack 🙇 Merging in. 👍 |
ritchie46
changed the title
Some(0)Some(0)
Closes: #5604
Changes:
Series::sum()on an empty series now evaluates toSome(0).Series::sum_as_series()on an empty series now returns a Series with a single zero'd value. This is done to enable the above change in both Rust and Python libraries.test_arithmetic_empty,test_arithmetic_null.I chose to add logic directly to
sum_as_series()as to reduce the blast radius of this new logic. Please let me know if any changes are needed, including location of tests!