feat(rust, python): allow arithmetic operations between numeric Series and list Series #18901

itamarst · 2024-09-24T15:37:12Z

Followup to #9188, allow arithmetic between:

Left-hand side list Series (including nesting!) with numeric leaf, and right-hand side numeric Series. Add/sub/mul/div/rem.
Right-hand numeric Series, and left-hand side list Series. This is limited to add and multiply since the other operation seemed kinda strange semantically, as they are not commutative. One could perhaps argue for subtraction, but lack of negation operator suggests maybe not.

itamarst · 2024-09-24T15:37:58Z

crates/polars-core/src/series/arithmetic/list_borrowed.rs

@@ -53,23 +53,112 @@ fn lists_same_shapes(left: &ArrayRef, right: &ArrayRef) -> bool {
    }
 }

+/// Arithmetic operations that can be applied to a Series


Is there some existing code that does this? There's a whole bunch of similar things but I may've missed an actual implementation.

itamarst · 2024-09-24T15:38:23Z

crates/polars-core/src/series/arithmetic/list_borrowed.rs

+
+impl Op {
+    /// Apply the operation to a pair of Series.
+    fn apply_with_series(&self, lhs: &Series, rhs: &Series) -> PolarsResult<Series> {


I tried to do these two functions as a single generic function and failed...

Why exactly? Seems this is possible with generics.

I couldn't figure out the constraints that work. I guess I can try another round and report back when I get stuck.

So the issue is that when you add two &Series, you get a PolarsResult<Series>, and when you add a &Series and u8 you get Series. Since the return types don't match, the constraints conflict (the former wants T: std:ops::Add<Output=PolarsResult<Series>>, the latter wants T: std:ops::Add<Output=Series>).

codecov · 2024-09-24T16:10:47Z

Codecov Report

Attention: Patch coverage is 99.09091% with 1 line in your changes missing coverage. Please review.

Project coverage is 79.77%. Comparing base (e186c03) to head (e916b56).
Report is 39 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/polars-plan/src/plans/aexpr/schema.rs	94.44%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #18901   +/-   ##
=======================================
  Coverage   79.77%   79.77%           
=======================================
  Files        1531     1531           
  Lines      208487   208536   +49     
  Branches     2913     2913           
=======================================
+ Hits       166314   166370   +56     
+ Misses      41622    41615    -7     
  Partials      551      551

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

codspeed-hq · 2024-09-24T16:14:29Z

CodSpeed Performance Report

Merging #18901 will not alter performance

_{Comparing itamarst:8006-list-arithmetic-part-2 (e916b56) with main (da8f358)}

Summary

✅ 40 untouched benchmarks

ritchie46 · 2024-09-24T17:01:24Z

Hey @itamarst Thanks for the PR. Before I review, can you rebase? There seem to be many commits from previous iteration.

itamarst · 2024-09-24T17:11:50Z

Will try.

…ic and the ls is a list.

itamarst · 2024-09-24T18:08:10Z

Looks like maybe one or two changes might be unnecessary, as reflected in the coverage report, so will make this draft while I fix that.

itamarst · 2024-09-24T18:37:28Z

OK, it's ready for review again. Not sure what's up with Windows failures.

itamarst · 2024-09-26T20:32:53Z

And it's both ready for review and green.

ritchie46 · 2024-09-27T11:41:27Z

crates/polars-core/src/series/arithmetic/borrowed.rs

+    let mut new_right_dtype = right_dtype.cast_leaf(leaf_super_dtype);
+
+    // If we have e.g. Array and List, we want to convert those too.
+    if (left_dtype.is_list() && right_dtype.is_array())


I don't think we should do that. I want those casts to be explicit for now.

By explicit do you mean "explicitly cast list to array" and vice versa as needed? This section is definitely necessary in some form, if I remove it:

[gw4] linux -- Python 3.12.3 /home/itamarst/devel/polars/.venv/bin/python3 tests/unit/datatypes/test_array.py:106: in test_array_list_supertype result = s1 == s2 polars/series/series.py:773: in __eq__ return self._comp(other, "eq") polars/series/series.py:753: in _comp return self._from_pyseries(getattr(self._s, op)(other._s)) E pyo3_runtime.PanicException: called `Result::unwrap()` on an `Err` value: SchemaMismatch(ErrString("invalid series dtype: expected `FixedSizeList`, got `list[f64]`")) -------------------------------- Captured stderr call --------------------------------thread '<unnamed>' panicked at crates/polars-core/src/series/comparison.rs:132:9: called `Result::unwrap()` on an `Err` value: SchemaMismatch(ErrString("invalid series dtype: expected `FixedSizeList`, got `list[f64]`")) note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace _______________________________ test_eq_array_cmp_list _______________________________[gw10] linux -- Python 3.12.3 /home/itamarst/devel/polars/.venv/bin/python3 tests/unit/series/test_equals.py:57: in test_eq_array_cmp_list result = s == [1, 2] polars/series/series.py:773: in __eq__ return self._comp(other, "eq") polars/series/series.py:753: in _comp return self._from_pyseries(getattr(self._s, op)(other._s)) E pyo3_runtime.PanicException: called `Result::unwrap()` on an `Err` value: SchemaMismatch(ErrString("invalid series dtype: expected `FixedSizeList`, got `list[i64]`"))

Yes, the user should cast explicitly. And we shuold return an error instead of panicking if they pass wrong types.

I can do that, but given this will involving changing the behavior of those two tests (from e.g. "these two things are equal" to "this now errors asking for a cast"), this is a backwards incompatible change. So as long as that's OK.

ritchie46 · 2024-09-27T11:44:14Z

crates/polars-core/src/series/arithmetic/list_borrowed.rs

+
+impl Op {
+    /// Apply the operation to a pair of Series.
+    fn apply_with_series(&self, lhs: &Series, rhs: &Series) -> PolarsResult<Series> {


Why exactly? Seems this is possible with generics.

nameexhaustion · 2024-10-01T13:59:03Z

Thanks for the PR - we want to try doing this with a more optimized kernel that operates directly on the primitive values - I will take it from here

…part-2

nameexhaustion · 2024-10-02T06:28:57Z

crates/polars-plan/src/plans/conversion/type_coercion/binary.rs

@@ -47,55 +47,6 @@ fn is_cat_str_binary(type_left: &DataType, type_right: &DataType) -> bool {
    }
 }

-fn process_list_arithmetic(


remove logic from the type_coercion optimizer - this should already be handled by get_arithmetic_field during conversion

nameexhaustion · 2024-10-02T06:36:43Z

crates/polars-core/src/series/arithmetic/list_borrowed.rs

+                Multiply => wrap!(lhs * rhs),
+                Divide => wrap!(lhs / rhs),
+                Remainder => wrap!(lhs % rhs),
+            }


I can make Op generic but it costs a little bit of sanity I think 😄

nameexhaustion · 2024-10-09T11:27:05Z

Closed in favor of #19162

itamarst requested review from ritchie46, c-peters, alexander-beedie, MarcoGorelli, reswqa and orlp as code owners September 24, 2024 15:37

github-actions bot added the title needs formatting label Sep 24, 2024

itamarst commented Sep 24, 2024

View reviewed changes

itamarst changed the title ~~feat: allow arithmetic operations between numeric Series and list Series~~ feat(rust, python): allow arithmetic operations between numeric Series and list Series Sep 24, 2024

pythonspeed added 5 commits September 24, 2024 13:59

Tests for arithmetic operations between list and numeric Series.

c1872cc

Cast the leaf dtype for nested dtypes.

944d4ae

Casting logic for mixed list and numeric dtypes.

e3141fd

Logic for arithmetic between a ListChunked and a numeric Series.

835f9fa

Handle addition and multipliction cases where the rhs Series is numer…

ef43739

…ic and the ls is a list.

itamarst force-pushed the 8006-list-arithmetic-part-2 branch from 4142998 to ef43739 Compare September 24, 2024 18:05

itamarst marked this pull request as draft September 24, 2024 18:08

pythonspeed added 2 commits September 24, 2024 14:09

Pacify pyright.

3ddfbc6

Revert unnecessary change.

9f984ee

itamarst marked this pull request as ready for review September 24, 2024 18:37

pythonspeed added 2 commits September 25, 2024 16:51

Merge remote-tracking branch 'origin/main' into 8006-take-2

2b115dc

Merge remote-tracking branch 'origin/main' into 8006-take-2

11a0d06

ritchie46 reviewed Sep 27, 2024

View reviewed changes

nameexhaustion added 4 commits October 2, 2024 15:45

Merge remote-tracking branch 'origin/main' into 8006-list-arithmetic-…

4000b0f

…part-2

fix incorrect arithmetic, remove type coercion

d25c9b8

make Op generic?

cbc2de1

remove unused

3d14608

nameexhaustion marked this pull request as draft October 2, 2024 06:23

nameexhaustion reviewed Oct 2, 2024

View reviewed changes

transmute_copy soundness

e916b56

nameexhaustion reviewed Oct 2, 2024

View reviewed changes

nameexhaustion closed this Oct 9, 2024

nameexhaustion mentioned this pull request Oct 14, 2024

test: Add more tests for list arithmetic #19225

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rust, python): allow arithmetic operations between numeric Series and list Series #18901

feat(rust, python): allow arithmetic operations between numeric Series and list Series #18901

itamarst commented Sep 24, 2024

itamarst Sep 24, 2024

itamarst Sep 24, 2024

ritchie46 Sep 27, 2024

itamarst Sep 27, 2024

itamarst Sep 27, 2024

codecov bot commented Sep 24, 2024 •

edited

Loading

codspeed-hq bot commented Sep 24, 2024 •

edited

Loading

ritchie46 commented Sep 24, 2024

itamarst commented Sep 24, 2024

itamarst commented Sep 24, 2024

itamarst commented Sep 24, 2024

itamarst commented Sep 26, 2024

ritchie46 Sep 27, 2024

itamarst Sep 27, 2024

ritchie46 Sep 27, 2024

itamarst Sep 27, 2024

ritchie46 Sep 27, 2024

nameexhaustion commented Oct 1, 2024

nameexhaustion Oct 2, 2024

nameexhaustion Oct 2, 2024

nameexhaustion commented Oct 9, 2024

feat(rust, python): allow arithmetic operations between numeric Series and list Series #18901

feat(rust, python): allow arithmetic operations between numeric Series and list Series #18901

Conversation

itamarst commented Sep 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 24, 2024 • edited Loading

Codecov Report

codspeed-hq bot commented Sep 24, 2024 • edited Loading

Merging #18901 will not alter performance

Summary

ritchie46 commented Sep 24, 2024

itamarst commented Sep 24, 2024

itamarst commented Sep 24, 2024

itamarst commented Sep 24, 2024

itamarst commented Sep 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nameexhaustion commented Oct 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nameexhaustion commented Oct 9, 2024

codecov bot commented Sep 24, 2024 •

edited

Loading

codspeed-hq bot commented Sep 24, 2024 •

edited

Loading