diff --git a/crates/polars-plan/src/dsl/function_expr/rolling.rs b/crates/polars-plan/src/dsl/function_expr/rolling.rs index 67772ef31adf..16e2c68b471a 100644 --- a/crates/polars-plan/src/dsl/function_expr/rolling.rs +++ b/crates/polars-plan/src/dsl/function_expr/rolling.rs @@ -126,7 +126,7 @@ fn convert<'a>( } pub(super) fn rolling_min(s: &Series, options: RollingOptions) -> PolarsResult { - s.rolling_min(options.clone().into()) + s.rolling_min(options.clone().try_into()?) } pub(super) fn rolling_min_by(s: &[Series], options: RollingOptions) -> PolarsResult { @@ -134,7 +134,7 @@ pub(super) fn rolling_min_by(s: &[Series], options: RollingOptions) -> PolarsRes } pub(super) fn rolling_max(s: &Series, options: RollingOptions) -> PolarsResult { - s.rolling_max(options.clone().into()) + s.rolling_max(options.clone().try_into()?) } pub(super) fn rolling_max_by(s: &[Series], options: RollingOptions) -> PolarsResult { @@ -142,7 +142,7 @@ pub(super) fn rolling_max_by(s: &[Series], options: RollingOptions) -> PolarsRes } pub(super) fn rolling_mean(s: &Series, options: RollingOptions) -> PolarsResult { - s.rolling_mean(options.clone().into()) + s.rolling_mean(options.clone().try_into()?) } pub(super) fn rolling_mean_by(s: &[Series], options: RollingOptions) -> PolarsResult { @@ -150,7 +150,7 @@ pub(super) fn rolling_mean_by(s: &[Series], options: RollingOptions) -> PolarsRe } pub(super) fn rolling_sum(s: &Series, options: RollingOptions) -> PolarsResult { - s.rolling_sum(options.clone().into()) + s.rolling_sum(options.clone().try_into()?) } pub(super) fn rolling_sum_by(s: &[Series], options: RollingOptions) -> PolarsResult { @@ -158,7 +158,7 @@ pub(super) fn rolling_sum_by(s: &[Series], options: RollingOptions) -> PolarsRes } pub(super) fn rolling_quantile(s: &Series, options: RollingOptions) -> PolarsResult { - s.rolling_quantile(options.clone().into()) + s.rolling_quantile(options.clone().try_into()?) } pub(super) fn rolling_quantile_by(s: &[Series], options: RollingOptions) -> PolarsResult { @@ -170,7 +170,7 @@ pub(super) fn rolling_quantile_by(s: &[Series], options: RollingOptions) -> Pola } pub(super) fn rolling_var(s: &Series, options: RollingOptions) -> PolarsResult { - s.rolling_var(options.clone().into()) + s.rolling_var(options.clone().try_into()?) } pub(super) fn rolling_var_by(s: &[Series], options: RollingOptions) -> PolarsResult { @@ -178,7 +178,7 @@ pub(super) fn rolling_var_by(s: &[Series], options: RollingOptions) -> PolarsRes } pub(super) fn rolling_std(s: &Series, options: RollingOptions) -> PolarsResult { - s.rolling_std(options.clone().into()) + s.rolling_std(options.clone().try_into()?) } pub(super) fn rolling_std_by(s: &[Series], options: RollingOptions) -> PolarsResult { diff --git a/crates/polars-time/src/chunkedarray/rolling_window/dispatch.rs b/crates/polars-time/src/chunkedarray/rolling_window/dispatch.rs index 787274b9c02a..9e785b0e7b36 100644 --- a/crates/polars-time/src/chunkedarray/rolling_window/dispatch.rs +++ b/crates/polars-time/src/chunkedarray/rolling_window/dispatch.rs @@ -48,8 +48,7 @@ where let arr = ca.downcast_iter().next().unwrap(); // "5i" is a window size of 5, e.g. fixed let arr = if options.window_size.parsed_int { - let options: RollingOptionsFixedWindow = options.into(); - check_input(options.window_size, options.min_periods)?; + let options: RollingOptionsFixedWindow = options.try_into()?; Ok(match ca.null_count() { 0 => rolling_agg_fn( @@ -78,7 +77,7 @@ where polars_ensure!(duration.duration_ns() > 0 && !duration.negative, ComputeError:"window size should be strictly positive"); let tu = options.tu.unwrap(); let by = options.by.unwrap(); - let closed_window = options.closed_window.expect("closed window must be set"); + let closed_window = options.closed_window.unwrap_or(ClosedWindow::Right); let func = rolling_agg_fn_dynamic.expect( "'rolling by' not yet supported for this expression, consider using 'group_by_rolling'", ); diff --git a/crates/polars-time/src/chunkedarray/rolling_window/mod.rs b/crates/polars-time/src/chunkedarray/rolling_window/mod.rs index 5eb03d2bcc0f..baad483a7f10 100644 --- a/crates/polars-time/src/chunkedarray/rolling_window/mod.rs +++ b/crates/polars-time/src/chunkedarray/rolling_window/mod.rs @@ -80,15 +80,22 @@ pub struct RollingOptionsImpl<'a> { pub fn_params: DynArgs, } -impl From for RollingOptionsImpl<'static> { - fn from(options: RollingOptions) -> Self { +impl TryFrom for RollingOptionsImpl<'static> { + type Error = PolarsError; + + fn try_from(options: RollingOptions) -> PolarsResult { let window_size = options.window_size; assert!( window_size.parsed_int, "should be fixed integer window size at this point" ); + polars_ensure!( + options.closed_window.is_none(), + InvalidOperation: "`closed_window` is not supported for fixed window size rolling aggregations, \ + consider using DataFrame.rolling for greater flexibility", + ); - RollingOptionsImpl { + Ok(RollingOptionsImpl { window_size, min_periods: options.min_periods, weights: options.weights, @@ -98,25 +105,34 @@ impl From for RollingOptionsImpl<'static> { tz: None, closed_window: None, fn_params: options.fn_params, - } + }) } } -impl From for RollingOptionsFixedWindow { - fn from(options: RollingOptions) -> Self { +impl TryFrom for RollingOptionsFixedWindow { + type Error = PolarsError; + + fn try_from(options: RollingOptions) -> PolarsResult { let window_size = options.window_size; assert!( window_size.parsed_int, "should be fixed integer window size at this point" ); + polars_ensure!( + options.closed_window.is_none(), + InvalidOperation: "`closed_window` is not supported for fixed window size rolling aggregations, \ + consider using DataFrame.rolling for greater flexibility", + ); + let window_size = window_size.nanoseconds() as usize; + check_input(window_size, options.min_periods)?; - RollingOptionsFixedWindow { - window_size: window_size.nanoseconds() as usize, + Ok(RollingOptionsFixedWindow { + window_size, min_periods: options.min_periods, weights: options.weights, center: options.center, fn_params: options.fn_params, - } + }) } } @@ -136,21 +152,27 @@ impl Default for RollingOptionsImpl<'static> { } } -impl<'a> From> for RollingOptionsFixedWindow { - fn from(options: RollingOptionsImpl<'a>) -> Self { +impl<'a> TryFrom> for RollingOptionsFixedWindow { + type Error = PolarsError; + fn try_from(options: RollingOptionsImpl<'a>) -> PolarsResult { let window_size = options.window_size; assert!( window_size.parsed_int, "should be fixed integer window size at this point" ); + polars_ensure!( + options.closed_window.is_none(), + InvalidOperation: "`closed_window` is not supported for fixed window size rolling aggregations, \ + consider using DataFrame.rolling for greater flexibility", + ); - RollingOptionsFixedWindow { + Ok(RollingOptionsFixedWindow { window_size: window_size.nanoseconds() as usize, min_periods: options.min_periods, weights: options.weights, center: options.center, fn_params: options.fn_params, - } + }) } } diff --git a/py-polars/polars/expr/expr.py b/py-polars/polars/expr/expr.py index 53f202b52572..641ff283325e 100644 --- a/py-polars/polars/expr/expr.py +++ b/py-polars/polars/expr/expr.py @@ -5717,7 +5717,7 @@ def rolling_min( *, center: bool = False, by: str | None = None, - closed: ClosedInterval = "right", + closed: ClosedInterval | None = None, warn_if_unsorted: bool = True, ) -> Self: """ @@ -5789,7 +5789,7 @@ def rolling_min( results will not be correct. closed : {'left', 'right', 'both', 'none'} Define which sides of the temporal interval are closed (inclusive); only - applicable if `by` has been set. + applicable if `by` has been set (in which case, it defaults to `'right'`). warn_if_unsorted Warn if data is not known to be sorted by `by` column (if passed). @@ -5929,7 +5929,7 @@ def rolling_max( *, center: bool = False, by: str | None = None, - closed: ClosedInterval = "right", + closed: ClosedInterval | None = None, warn_if_unsorted: bool = True, ) -> Self: """ @@ -5997,7 +5997,7 @@ def rolling_max( be of dtype Datetime or Date. closed : {'left', 'right', 'both', 'none'} Define which sides of the temporal interval are closed (inclusive); only - applicable if `by` has been set. + applicable if `by` has been set (in which case, it defaults to `'right'`). warn_if_unsorted Warn if data is not known to be sorted by `by` column (if passed). @@ -6166,7 +6166,7 @@ def rolling_mean( *, center: bool = False, by: str | None = None, - closed: ClosedInterval = "right", + closed: ClosedInterval | None = None, warn_if_unsorted: bool = True, ) -> Self: """ @@ -6238,7 +6238,7 @@ def rolling_mean( results will not be correct. closed : {'left', 'right', 'both', 'none'} Define which sides of the temporal interval are closed (inclusive); only - applicable if `by` has been set. + applicable if `by` has been set (in which case, it defaults to `'right'`). warn_if_unsorted Warn if data is not known to be sorted by `by` column (if passed). @@ -6413,7 +6413,7 @@ def rolling_sum( *, center: bool = False, by: str | None = None, - closed: ClosedInterval = "right", + closed: ClosedInterval | None = None, warn_if_unsorted: bool = True, ) -> Self: """ @@ -6481,7 +6481,7 @@ def rolling_sum( of dtype `{Date, Datetime}` closed : {'left', 'right', 'both', 'none'} Define which sides of the temporal interval are closed (inclusive); only - applicable if `by` has been set. + applicable if `by` has been set (in which case, it defaults to `'right'`). warn_if_unsorted Warn if data is not known to be sorted by `by` column (if passed). @@ -6650,7 +6650,7 @@ def rolling_std( *, center: bool = False, by: str | None = None, - closed: ClosedInterval = "right", + closed: ClosedInterval | None = None, ddof: int = 1, warn_if_unsorted: bool = True, ) -> Self: @@ -6720,7 +6720,7 @@ def rolling_std( results will not be correct. closed : {'left', 'right', 'both', 'none'} Define which sides of the temporal interval are closed (inclusive); only - applicable if `by` has been set. + applicable if `by` has been set (in which case, it defaults to `'right'`). ddof "Delta Degrees of Freedom": The divisor for a length N window is N - ddof warn_if_unsorted @@ -6898,7 +6898,7 @@ def rolling_var( *, center: bool = False, by: str | None = None, - closed: ClosedInterval = "right", + closed: ClosedInterval | None = None, ddof: int = 1, warn_if_unsorted: bool = True, ) -> Self: @@ -6967,7 +6967,7 @@ def rolling_var( results will not be correct. closed : {'left', 'right', 'both', 'none'} Define which sides of the temporal interval are closed (inclusive); only - applicable if `by` has been set. + applicable if `by` has been set (in which case, it defaults to `'right'`). ddof "Delta Degrees of Freedom": The divisor for a length N window is N - ddof warn_if_unsorted @@ -7145,7 +7145,7 @@ def rolling_median( *, center: bool = False, by: str | None = None, - closed: ClosedInterval = "right", + closed: ClosedInterval | None = None, warn_if_unsorted: bool = True, ) -> Self: """ @@ -7214,7 +7214,7 @@ def rolling_median( results will not be correct. closed : {'left', 'right', 'both', 'none'} Define which sides of the temporal interval are closed (inclusive); only - applicable if `by` has been set. + applicable if `by` has been set (in which case, it defaults to `'right'`). warn_if_unsorted Warn if data is not known to be sorted by `by` column (if passed). @@ -7305,7 +7305,7 @@ def rolling_quantile( *, center: bool = False, by: str | None = None, - closed: ClosedInterval = "right", + closed: ClosedInterval | None = None, warn_if_unsorted: bool = True, ) -> Self: """ @@ -7377,7 +7377,7 @@ def rolling_quantile( results will not be correct. closed : {'left', 'right', 'both', 'none'} Define which sides of the temporal interval are closed (inclusive); only - applicable if `by` has been set. + applicable if `by` has been set (in which case, it defaults to `'right'`). warn_if_unsorted Warn if data is not known to be sorted by `by` column (if passed). diff --git a/py-polars/tests/unit/operations/rolling/test_rolling.py b/py-polars/tests/unit/operations/rolling/test_rolling.py index 6ac4a5937a12..8eafa8e4e32f 100644 --- a/py-polars/tests/unit/operations/rolling/test_rolling.py +++ b/py-polars/tests/unit/operations/rolling/test_rolling.py @@ -8,7 +8,7 @@ from numpy import nan import polars as pl -from polars.exceptions import ComputeError +from polars.exceptions import ComputeError, InvalidOperationError from polars.testing import assert_frame_equal, assert_series_equal if TYPE_CHECKING: @@ -224,6 +224,12 @@ def test_rolling_infinity() -> None: assert_series_equal(s, expected) +def test_rolling_invalid_closed_option() -> None: + df = pl.DataFrame({"a": [4, 5, 6]}).sort("a") + with pytest.raises(InvalidOperationError, match="consider using DataFrame.rolling"): + df.with_columns(pl.col("a").rolling_sum(2, closed="left")) + + def test_rolling_extrema() -> None: # sorted data and nulls flags trigger different kernels df = (