simplify algebra in Year offset apply methods #18280
Conversation
Codecov Report
@@ Coverage Diff @@
## master #18280 +/- ##
==========================================
- Coverage 91.39% 91.37% -0.03%
==========================================
Files 164 164
Lines 49854 49808 -46
==========================================
- Hits 45566 45511 -55
- Misses 4288 4297 +9
Continue to review full report at Codecov.
|
pandas/_libs/tslibs/offsets.pyx
Outdated
| @@ -428,3 +428,36 @@ cpdef datetime shift_month(datetime stamp, int months, object day_opt=None): | |||
| else: | |||
| raise ValueError(day_opt) | |||
| return stamp.replace(year=year, month=month, day=day) | |||
|
|
|||
|
|
|||
| cpdef int _get_day_of_month(datetime other, day_opt): | |||
There was a problem hiding this comment.
add doc-string (examples too!)
pandas/_libs/tslibs/offsets.pyx
Outdated
| Possibly increment or decrement the number of periods to shift | ||
| based on rollforward/rollbackward conventions. | ||
|
|
||
| Mirrors `roll_check` in tslib.shift_months |
As in #18263, I think so, yes. |
|
are there unit tests for these (can you point)? in particular the raising of get_day_of_month? |
| -6 | ||
|
|
||
| """ | ||
| # Note: The other.day < ... condition will never hold when day_opt=='start' |
There was a problem hiding this comment.
theoretically you should assert day_opt here (because if n=0 it is not checked)
There was a problem hiding this comment.
You mean assert that it is valid? Sure.
BTW good call on testing the raising get_day_of_month. The exception gets ignored without an except -1.
There aren't any specific tests for these. I'll add some. |
|
yeah i suspect lots of things are not unit tested so taking this opportunity to put in place :) |
|
Sounds good. Another BTW: we can get coverage for _libs: #12634 (comment) |
jbrockmendel
force-pushed
the
tslibs-offsets-years
branch
from
8e43700 to
369da47
Compare
|
thanks! I love unit tests & doc-strings! |
This PR is careful to only touch YearEnd and YearBegin classes, so should not overlap with #18263 or #18278. But like 18278, once the algebra is simplified, it looks an awful lot like 18263.
The bad news is that the process of simplifying the dynamically-defined
_increment,_decrement,_rollffunctions is no fun, and I'm not aware of any good way to "show your work" to walk through the in-between steps.git diff upstream/master -u -- "*.py" | flake8 --diff