fix(rust, python): std when ddof>=n_values returns None even in rolling context #11750

MarcoGorelli · 2023-10-15T20:04:40Z

~~need to make the same change in the group_by_rolling(...).agg case as that's being tested against~~ done

…ng context

MarcoGorelli · 2023-10-16T09:23:20Z

py-polars/tests/parametric/test_groupby_rolling.py

-            #  "std", blocked by https://github.com/pola-rs/polars/issues/11140
-            #  "var", blocked by https://github.com/pola-rs/polars/issues/11140
+            "std",
+            "var",


woohoo finally

ritchie46 · 2023-10-16T17:51:04Z

crates/polars-arrow/src/legacy/kernels/rolling/no_nulls/mean.rs

-        let sum = self.sum.update(start, end);
-        sum / NumCast::from(end - start).unwrap()
+    unsafe fn update(&mut self, start: usize, end: usize) -> Option<T> {
+        let sum = self.sum.update(start, end).unwrap();


can unwrap_unchecked this one as we know we always get value from sum.

ritchie46 · 2023-10-16T17:54:39Z

crates/polars-arrow/src/legacy/kernels/rolling/no_nulls/mod.rs

@@ -47,21 +47,36 @@ where
    let len = values.len();
    let (start, end) = det_offsets_fn(0, window_size, len);
    let mut agg_window = Agg::new(values, start, end, params);
+    let mut validity = match create_validity(min_periods, len, window_size, &det_offsets_fn) {


Can you use unwrap_or_else here? That elides the Some branch implicitly.

ritchie46 · 2023-10-16T17:56:02Z

crates/polars-arrow/src/legacy/kernels/rolling/no_nulls/mod.rs


    let out = (0..len)
        .map(|idx| {
            let (start, end) = det_offsets_fn(idx, window_size, len);
            // safety:
            // we are in bounds
-            unsafe { agg_window.update(start, end) }
+            let agg = unsafe { agg_window.update(start, end) };
+            match agg {


unwrap_or_else

ritchie46 · 2023-10-16T17:59:06Z

crates/polars-arrow/src/legacy/kernels/rolling/no_nulls/mod.rs

    Ok(Box::new(PrimitiveArray::new(
        T::PRIMITIVE.into(),
        out.into(),
-        validity.map(|b| b.into()),
+        Some(validity.into()),


I don't like we always create a validity even if we don't have any nulls. I think we should branch on the case where create_validity

returns a validity (e.g all null). In that case all values are null and we can directly return and create a Vec<T::zero> for the masked out values.

doesn't return a validity. Then we can collect a PrimitiveArray from an iterator over Option<T>

Is the validity only not necessary if there are no nulls?

Because here, even if there were no nulls to begin with and even if min_periods was 0, agg_window.update(start, end) might introduce nulls (for example, if taking the std of a window whose length is smaller than ddof)

ritchie46 · 2023-10-16T17:59:57Z

crates/polars-arrow/src/legacy/kernels/rolling/no_nulls/quantile.rs

                } else {
                    let proportion = T::from(float_idx - idx as f64).unwrap();
-                    proportion * (vals[top_idx] - vals[idx]) + vals[idx]
+                    Some(proportion * (vals[top_idx] - vals[idx]) + vals[idx])


Can we make these get_unchecked_release?

…ling

stinodego · 2024-02-08T12:15:10Z

@MarcoGorelli this PR is pretty old and still not marked as reviewable - do you need help finishing this or can it perhaps be closed?

MarcoGorelli · 2024-02-08T12:19:46Z

Thanks for the ping - will pick this up again, hopefully now I'll be able to address it (if not, will ask for help)

…ling

codecov · 2024-03-01T12:14:01Z

Codecov Report

Attention: Patch coverage is 75.38462% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 81.01%. Comparing base (0a56aa4) to head (dceff5b).

Files	Patch %	Lines
...polars-core/src/frame/group_by/aggregations/mod.rs	38.46%	8 Missing ⚠️
...s-arrow/src/legacy/kernels/rolling/no_nulls/mod.rs	71.42%	4 Missing ⚠️
...ow/src/legacy/kernels/rolling/no_nulls/quantile.rs	50.00%	3 Missing ⚠️
...ow/src/legacy/kernels/rolling/no_nulls/variance.rs	92.30%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #11750      +/-   ##
==========================================
- Coverage   81.02%   81.01%   -0.01%     
==========================================
  Files        1332     1332              
  Lines      172865   172879      +14     
  Branches     2458     2458              
==========================================
+ Hits       140059   140064       +5     
- Misses      32339    32347       +8     
- Partials      467      468       +1

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

…ling

ritchie46 · 2024-03-08T14:46:13Z

crates/polars-arrow/src/legacy/kernels/rolling/no_nulls/mod.rs

+    if let Some(validity) = create_validity(min_periods, len, window_size, &det_offsets_fn) {
+        if validity.iter().all(|x| !x) {
+            // all null!
+            return Ok(Box::new(PrimitiveArray::new(


We should use new_null constructor here. The bitmap now first allocates a vec full of false and then iterates that to construct a bitmap. That can be done in a single allocation.

…ling

ritchie46

Thank you Marco. Great one.

github-actions bot added fix Bug fix python Related to Python Polars rust Related to Rust Polars labels Oct 15, 2023

This comment was marked as resolved.

Sign in to view

MarcoGorelli added 2 commits October 16, 2023 11:46

fix(rust, python): std when ddof>=n_values returns None even in rolli…

39c6c7d

…ng context

doctests

8254439

MarcoGorelli force-pushed the ddof-le-nvalues-rolling branch from 5c4dbbd to 39c6c7d Compare October 16, 2023 09:05

MarcoGorelli changed the title ~~fix(rust, python): single-element std with ddof=1 now returns None in rolling setting~~ fix(rust, python): std when ddof>=n_values returns None even in rolling context Oct 16, 2023

MarcoGorelli commented Oct 16, 2023

View reviewed changes

MarcoGorelli marked this pull request as ready for review October 16, 2023 09:38

MarcoGorelli requested review from ritchie46, stinodego, alexander-beedie and orlp as code owners October 16, 2023 09:38

ritchie46 requested changes Oct 16, 2023

View reviewed changes

MarcoGorelli marked this pull request as draft October 22, 2023 14:33

MarcoGorelli added 6 commits November 12, 2023 16:02

Merge remote-tracking branch 'upstream/main' into ddof-le-nvalues-rol…

a3a3346

…ling

wip

1f6ed72

chore(rust): use unwrap_or_else

a5c4fe8

use unwrap_or_else

1c29190

use get_unchecked_release

d65169a

use get_unchecked_release

8ded05a

MarcoGorelli mentioned this pull request Nov 12, 2023

chore(rust): use unwrap_or_else and get_unchecked_release in rolling kernels #12405

Merged

MarcoGorelli added 5 commits November 13, 2023 08:10

Merge remote-tracking branch 'upstream/main' into ddof-le-nvalues-rol…

cd1d340

…ling

Merge remote-tracking branch 'upstream/main' into ddof-le-nvalues-rol…

1d4a23f

…ling

Merge remote-tracking branch 'upstream/main' into ddof-le-nvalues-rol…

5ccd6f4

…ling

rust tests

617421e

Merge remote-tracking branch 'upstream/main' into ddof-le-nvalues-rol…

c3dc96a

…ling

MarcoGorelli marked this pull request as ready for review December 30, 2023 14:14

MarcoGorelli requested a review from c-peters as a code owner December 30, 2023 14:14

MarcoGorelli marked this pull request as draft December 30, 2023 16:53

MarcoGorelli added 3 commits March 1, 2024 09:36

Merge remote-tracking branch 'upstream/main' into ddof-le-nvalues-rol…

5f98db4

…ling

dont create validity if you dont have to

b0eef23

make doctest

74b07d8

MarcoGorelli marked this pull request as ready for review March 1, 2024 11:23

MarcoGorelli requested a review from ritchie46 March 1, 2024 12:38

MarcoGorelli added 2 commits March 5, 2024 09:25

Merge remote-tracking branch 'upstream/main' into ddof-le-nvalues-rol…

788221b

…ling

Merge remote-tracking branch 'upstream/main' into ddof-le-nvalues-rol…

30fd107

…ling

ritchie46 reviewed Mar 8, 2024

View reviewed changes

MarcoGorelli added 3 commits March 8, 2024 14:59

Merge remote-tracking branch 'upstream/main' into ddof-le-nvalues-rol…

eae7780

…ling

use new_null

27f4914

Merge remote-tracking branch 'upstream/main' into ddof-le-nvalues-rol…

dceff5b

…ling

ritchie46 approved these changes Mar 8, 2024

View reviewed changes

ritchie46 merged commit 6b23f79 into pola-rs:main Mar 8, 2024
24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(rust, python): std when ddof>=n_values returns None even in rolling context #11750

fix(rust, python): std when ddof>=n_values returns None even in rolling context #11750

MarcoGorelli commented Oct 15, 2023 •

edited

Loading

This comment was marked as resolved.

MarcoGorelli Oct 16, 2023

ritchie46 Oct 16, 2023

ritchie46 Oct 16, 2023

ritchie46 Oct 16, 2023

ritchie46 Oct 16, 2023

MarcoGorelli Nov 12, 2023

ritchie46 Oct 16, 2023

stinodego commented Feb 8, 2024

MarcoGorelli commented Feb 8, 2024

codecov bot commented Mar 1, 2024 •

edited

Loading

ritchie46 Mar 8, 2024

ritchie46 left a comment

fix(rust, python): std when ddof>=n_values returns None even in rolling context #11750

fix(rust, python): std when ddof>=n_values returns None even in rolling context #11750

Conversation

MarcoGorelli commented Oct 15, 2023 • edited Loading

This comment was marked as resolved.

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stinodego commented Feb 8, 2024

MarcoGorelli commented Feb 8, 2024

codecov bot commented Mar 1, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

ritchie46 left a comment

Choose a reason for hiding this comment

MarcoGorelli commented Oct 15, 2023 •

edited

Loading

codecov bot commented Mar 1, 2024 •

edited

Loading