DEPR: deprecate string H, BH, CBH in offsets frequencies, resolution abbreviations, _attrname_to_abbrevs #54939
Conversation
pandas/_libs/tslibs/offsets.pyx
Outdated
| if self._prefix.startswith(("C", "c")): | ||
| # CustomBusinessHour | ||
| return CustomBusinessDay( |
There was a problem hiding this comment.
should this one check for both? if it's business hour, don't we know it's going to start with 'c'?
There was a problem hiding this comment.
Thank you for reviewing this PR.
I think we need to check both: upper and lowercase 'c', because we added "cbh" to the dictionary _dont_uppercase = {"h", "bh", "cbh", "MS", "ms", "s", "me"}.
On the other hand, if freq is not in _dont_uppercase we do uppercasing in _get_offset().
When I remove the check for "c", tests for CustomBusinessHour fail.
There was a problem hiding this comment.
thanks for explaining - the comment
# CustomBusinessHour
return CustomBusinessDaymight be a bit misleading, but I'll pull your code tomorrow, step through it, and check
the rest looks good though, so we should be very close
There was a problem hiding this comment.
I had another look isn't _prefix always 'cbh' for CustomBusinessHour?
If so, then I think just checking if self._prefix.startswith("c"): should be enough? At least, if I do that, then pytest pandas/tests/tseries/offsets/ all passes
There was a problem hiding this comment.
Thanks, I agree, checking if self._prefix.startswith("c"): should be enough. I changed the condition, locally all tests passed.
|
I added tests for Hour and CustomBusinessHour and corrected the definition of |
pandas/_libs/tslibs/offsets.pyx
Outdated
| if prefix in c_DEPR_ABBREVS: | ||
| warnings.warn( | ||
| f"\'{prefix}\' is deprecated and will be removed " | ||
| f"in a future version. Please use " | ||
| f"\'{c_DEPR_ABBREVS.get(prefix)}\' " | ||
| f"instead of \'{prefix}\'.", | ||
| FutureWarning, | ||
| stacklevel=find_stack_level(), | ||
| ) | ||
| prefix = c_DEPR_ABBREVS[prefix] |
There was a problem hiding this comment.
could you please explain why this needed to be moved here, from line 4642?
There was a problem hiding this comment.
I moved the check and transformation prefix = c_DEPR_ABBREVS[prefix] to keep it together with the call _get_offset in line 4666, where we use the new prefix.
I think it's not necessary, I can move it back.
There was a problem hiding this comment.
It was my mistake, I moved the check if prefix in c_DEPR_ABBREVS: back. Seems like we don't have a test to cover this case ,I added a test in PR "DEPR: 'A' in favour of 'Y'" that fails if the order of checks isn't correct.
pandas/_libs/tslibs/offsets.pyx
Outdated
| offset = _get_offset(name) | ||
| offset = _get_offset(prefix) |
There was a problem hiding this comment.
we have tests that didn't work for BH / CBH because we changed prefix to bh / cbh in line 4663 prefix = c_DEPR_ABBREVS[prefix], but then passed to _get_offset name, = BH / CBH. That is why I replaced name with prefix.
|
looks like we're close, just got some conflicts from the A -> Y one (but I think you've mastered resolving conflicts now, so should be fine 😄 ) |
thanks, I resolved conflicts, ci is green. |
xref #54061