-
Notifications
You must be signed in to change notification settings - Fork 908
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
Extend TimeSeries.resample() to support more resampling methods #2643
Conversation
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.
Nice contribution @jonasblanc, some minor comments to improve the readibility
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2643 +/- ##
==========================================
- Coverage 94.20% 94.17% -0.04%
==========================================
Files 141 141
Lines 15491 15493 +2
==========================================
- Hits 14594 14591 -3
- Misses 897 902 +5 ☔ View full report in Codecov by Sentry. |
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.
Very nice PR, thanks and great job @jonasblanc 🚀 Just had some minor suggestions, after that we're ready to merge :)
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.
LGTM, great first contribution @jonasblanc! 🚀
Checklist before merging this PR:
Fixes #699
Summary
TimeSeries.resample()
.method_args
.Other Information
asfreq
is used for down-sampling.any()
andall()
aggregation methods, to be consistent withTimeSeries
, I castbool
back toint
after the aggregation.pad
as the default method so as not to break the default behavior, even though it's not the most intuitive method for down-sampling.