feat: raise more informative error messages in rolling_* aggregations instead of panicking #15979
Conversation
github-actions
bot
added
enhancement
… instead of panicking
| impl TryFrom<RollingOptions> for RollingOptionsImpl<'static> { | ||
| type Error = PolarsError; | ||
|
|
||
| fn try_from(options: RollingOptions) -> PolarsResult<Self> { | ||
| 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", | ||
| ); | ||
|
|
||
| Ok(RollingOptionsImpl { | ||
| window_size, | ||
| impl From<RollingOptions> for RollingOptionsImpl<'static> { |
There was a problem hiding this comment.
make this from, instead of try_from, so the validation is done when going from RollingOptionsImpl to RollingOptionsFixedWindow (or to RollingOptionsDynamicWindow)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15979 +/- ##
==========================================
+ Coverage 80.90% 80.91% +0.01%
==========================================
Files 1384 1384
Lines 178196 178195 -1
Branches 3050 3050
==========================================
+ Hits 144173 144191 +18
+ Misses 33535 33518 -17
+ Partials 488 486 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| let arr = if options.window_size.parsed_int { | ||
| let arr = if options.by.is_none() { |
There was a problem hiding this comment.
rather than branching based on window_size.parsed_int (which can lead to some surprising error messages), I think it's cleaner to branch on by.is_none
MarcoGorelli
requested review from
ritchie46,
stinodego,
c-peters,
alexander-beedie,
reswqa and
orlp
as code owners
| @@ -130,39 +126,39 @@ fn convert<'a>( | |||
| } | |||
|
|
|||
| pub(super) fn rolling_min(s: &Series, options: RollingOptions) -> PolarsResult<Series> { | |||
| s.rolling_min(options.clone().try_into()?) | |||
| s.rolling_min(options.clone().into()) | |||
There was a problem hiding this comment.
Why do we clone here? We have town the options.
There was a problem hiding this comment.
no reason as far as I can tell, I've removed it
… instead of panicking (pola-rs#15979)
… instead of panicking (pola-rs#15979)
closes #15977
The validation of params is currently a bit messy, I'm suggesting to do, within
rolling_agg:This will also make it easier to split out
rolling_*fromrolling_*_by