frequencies: Calculate all pivot points based on end pivot #1150
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1150 +/- ##
=======================================
Coverage 68.19% 68.19%
=======================================
Files 63 63
Lines 6748 6748
Branches 1654 1654
=======================================
Hits 4602 4602
Misses 1838 1838
Partials 308 308
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
This looks great, @victorlin! Could we mention somewhere that this fix is necessary because of the nature of Python's various timedelta implementation (i.e., that pivot = pivot - delta iteratively applied will produce a different result than pivot = end - delta * len(pivots)? It's a subtle point that might have been documented in other standard Python tools, but it would be useful to document for ourselves, too.
This shows the current behavior, to be changed by the following commit.
Previously, monthly pivots calculated from the end of a month failed to account for the variation in the number of days per month. This was due to subtle differences between subtracting an isodate.Duration against a decremental date (previous implementation) and subtracting an isodate.Duration with a multiplier against a fixed date (new implementation). Tests updated with the new behavior.
victorlin
force-pushed
the
victorlin/fix-frequencies-pivots
branch
from
16776ab to
004d017
Compare
@huddlej I added more explanation in the commit message (dde579b). Does that look better? |
There was a problem hiding this comment.
Thank you for the updated commit message, @victorlin. This looks ready to merge!
Description of proposed changes
Previously, monthly pivots calculated from the end of a month failed to account for the variation in the number of days per month.
Related issue(s)
Fixes #963
Testing
Checklist