-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
DEPR: lower/uppercase strings such as 'y', 'q', 'H', 'MIN', etc. denoting freqs/units for time series, period, and timedelta #56346
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
Changes from 24 commits
c7d82ec
614710b
458a9c1
e9c8af5
73f68f0
eea9b69
386eb57
5dcb918
25bfbef
eb418eb
943b898
435db76
3cc94d2
457ae96
3837b7f
f709557
7cd909f
8d1d6dd
6c24541
286b618
606d0c5
32993a9
dcf1994
5443b1e
7889c7d
00e80c0
0eb098e
b6074b1
530f056
f600538
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1766,7 +1766,7 @@ cdef class BusinessDay(BusinessMixin): | |
s -= hrs * 3600 | ||
mts = int(s / 60) | ||
if mts != 0: | ||
off_str += str(mts) + "Min" | ||
off_str += str(mts) + "min" | ||
s -= mts * 60 | ||
if s != 0: | ||
off_str += str(s) + "s" | ||
|
@@ -4654,36 +4654,13 @@ _lite_rule_alias = { | |
"BYE": "BYE-DEC", # BYearEnd(month=12), | ||
"BYS": "BYS-JAN", # BYearBegin(month=1), | ||
|
||
"Min": "min", | ||
"min": "min", | ||
"ms": "ms", | ||
"us": "us", | ||
"ns": "ns", | ||
} | ||
|
||
_dont_uppercase = { | ||
"h", | ||
"bh", | ||
"cbh", | ||
"MS", | ||
"ms", | ||
"s", | ||
"me", | ||
"qe", | ||
"qe-dec", | ||
"qe-jan", | ||
"qe-feb", | ||
"qe-mar", | ||
"qe-apr", | ||
"qe-may", | ||
"qe-jun", | ||
"qe-jul", | ||
"qe-aug", | ||
"qe-sep", | ||
"qe-oct", | ||
"qe-nov", | ||
"ye", | ||
} | ||
_dont_uppercase = {"h", "bh", "cbh", "MS", "ms", "s"} | ||
|
||
|
||
INVALID_FREQ_ERR_MSG = "Invalid frequency: {0}" | ||
|
@@ -4702,7 +4679,29 @@ def _get_offset(name: str) -> BaseOffset: | |
-------- | ||
_get_offset('EOM') --> BMonthEnd(1) | ||
""" | ||
if name.lower() not in _dont_uppercase: | ||
if ( | ||
name not in _lite_rule_alias | ||
and (name.upper() in _lite_rule_alias) | ||
and name != "ms" | ||
): | ||
warnings.warn( | ||
f"\'{name}\' is deprecated and will be removed " | ||
f"in a future version, please use \'{name.upper()}\' instead.", | ||
FutureWarning, | ||
stacklevel=find_stack_level(), | ||
) | ||
elif ( | ||
name not in _lite_rule_alias | ||
and (name.lower() in _lite_rule_alias) | ||
and name != "MS" | ||
): | ||
warnings.warn( | ||
f"\'{name}\' is deprecated and will be removed " | ||
f"in a future version, please use \'{name.lower()}\' instead.", | ||
FutureWarning, | ||
stacklevel=find_stack_level(), | ||
) | ||
if name not in _dont_uppercase: | ||
name = name.upper() | ||
name = _lite_rule_alias.get(name, name) | ||
name = _lite_rule_alias.get(name.lower(), name) | ||
|
@@ -4795,40 +4794,60 @@ cpdef to_offset(freq, bint is_period=False): | |
|
||
tups = zip(split[0::4], split[1::4], split[2::4]) | ||
for n, (sep, stride, name) in enumerate(tups): | ||
if is_period is False and name in c_OFFSET_DEPR_FREQSTR: | ||
if is_period is False and name.upper() in c_OFFSET_DEPR_FREQSTR: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also, little nitpick, but There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks, |
||
warnings.warn( | ||
f"\'{name}\' is deprecated and will be removed " | ||
f"in a future version, please use " | ||
f"\'{c_OFFSET_DEPR_FREQSTR.get(name)}\' instead.", | ||
f"\'{c_OFFSET_DEPR_FREQSTR.get(name.upper())}\' instead.", | ||
FutureWarning, | ||
stacklevel=find_stack_level(), | ||
) | ||
name = c_OFFSET_DEPR_FREQSTR[name] | ||
if is_period is True and name in c_REVERSE_OFFSET_DEPR_FREQSTR: | ||
if name.startswith("Y"): | ||
name = c_OFFSET_DEPR_FREQSTR[name.upper()] | ||
elif (is_period is False and | ||
name != name.upper() and | ||
name.upper() in c_REVERSE_OFFSET_DEPR_FREQSTR): | ||
warnings.warn( | ||
f"\'{name}\' is deprecated and will be removed " | ||
f"in a future version, please use " | ||
f"\'{name.upper()}\' instead.", | ||
FutureWarning, | ||
stacklevel=find_stack_level(), | ||
) | ||
name = name.upper() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the idea here that if someone passes I'm not sure this is the right place for this warning to be honest - like this it'll only show the warning for lowercase versions of aliases which have been renamed. For example: In [5]: to_offset('qs')
Out[5]: <QuarterBegin: startingMonth=1> doesn't emit any warning, and similarly for others which aren't part of that list There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thank you for noticing this, it's my mistake. I fixed it and updated the PR. |
||
if is_period is True and name.upper() in c_REVERSE_OFFSET_DEPR_FREQSTR: | ||
if name.upper().startswith("Y"): | ||
raise ValueError( | ||
f"for Period, please use \'Y{name[2:]}\' " | ||
f"for Period, please use \'Y{name.upper()[2:]}\' " | ||
f"instead of \'{name}\'" | ||
) | ||
if (name.startswith("B") or | ||
name.startswith("S") or name.startswith("C")): | ||
if (name.upper().startswith("B") or | ||
name.upper().startswith("S") or | ||
name.upper().startswith("C")): | ||
raise ValueError(INVALID_FREQ_ERR_MSG.format(name)) | ||
else: | ||
raise ValueError( | ||
f"for Period, please use " | ||
f"\'{c_REVERSE_OFFSET_DEPR_FREQSTR.get(name)}\' " | ||
f"\'{c_REVERSE_OFFSET_DEPR_FREQSTR.get(name.upper())}\' " | ||
f"instead of \'{name}\'" | ||
) | ||
elif is_period is True and name in c_OFFSET_DEPR_FREQSTR: | ||
if name.startswith("A"): | ||
elif is_period is True and name.upper() in c_OFFSET_DEPR_FREQSTR: | ||
if name.upper().startswith("A"): | ||
warnings.warn( | ||
f"\'{name}\' is deprecated and will be removed in a future " | ||
f"version, please use \'{c_DEPR_ABBREVS.get(name)}\' " | ||
f"version, please use " | ||
f"\'{c_DEPR_ABBREVS.get(name.upper())}\' instead.", | ||
FutureWarning, | ||
stacklevel=find_stack_level(), | ||
) | ||
if name.upper() != name: | ||
warnings.warn( | ||
f"\'{name}\' is deprecated and will be removed in " | ||
f"a future version, please use \'{name.upper()}\' " | ||
f"instead.", | ||
FutureWarning, | ||
stacklevel=find_stack_level(), | ||
) | ||
name = c_OFFSET_DEPR_FREQSTR.get(name) | ||
name = c_OFFSET_DEPR_FREQSTR.get(name.upper()) | ||
|
||
if sep != "" and not sep.isspace(): | ||
raise ValueError("separator must be spaces") | ||
|
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.
separate to this PR, but can we move
'ms'
,'us'
,'ns'
, and'min'
from_lite_rule_alias
?_lit_rule_alias
is only ever used with.get(name, name)
, so I don't think they need to be in it?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.
Thank you, I did as you suggested and removed
"min", ”ms”, “ns", and “us"
from_lite_rule_alias
in separate PR #56516. I added these aliases to the list _dont_uppercase, because otherwise we uppercase them in_get_offset()
. could you please take a look at this PR?I am not sure, do we need “
Min”
in_lite_rule_alias
? We want to deprecate the alias “Min” and left only lowercase “min” for Minutes. Am I correct?