-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Arguments for upsample
only have to be sorted within groups
#18264
fix: Arguments for upsample
only have to be sorted within groups
#18264
Conversation
In the case of `group_by` being provide as an extra argument to `upsample`, the index needs to be only sorted within those groups.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #18264 +/- ##
=======================================
Coverage 80.21% 80.22%
=======================================
Files 1500 1500
Lines 198897 198897
Branches 2837 2837
=======================================
+ Hits 159547 159566 +19
+ Misses 38822 38803 -19
Partials 528 528 ☔ View full report in Codecov by Sentry. |
Simplify the testing for the newly added tests to only test newly added functionality.
crates/polars-time/src/upsample.rs
Outdated
s.ensure_sorted_arg("upsample")?; | ||
if by.is_empty() { | ||
s.ensure_sorted_arg("upsample")?; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for your PR!
does it work to move this check into upsample_single_impl
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! That was indeed the cleaner way to do it.
…ork_if_args_are_sorted_within_group
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks!
In the case of
group_by
being provide as an extra argument toupsample
, the index needs to be only sorted within those groups. This tries to fix the issue observed in #18229.Tests are added to check that
Note, this is my first commit and I am not a Rust developer.