DEPR: depreecate uppercase units 'MIN', 'MS', 'US', 'NS' in Timedelta and 'to_timedelta' #57700
Conversation
| FutureWarning, | ||
| stacklevel=find_stack_level(), | ||
| ) | ||
| unit = unit.lower() | ||
| elif unit in c_DEPR_ABBREVS: |
There was a problem hiding this comment.
Instead of adding another branch is there any reason to not just add these to c_DEPR_ABBREVS?
There was a problem hiding this comment.
thank you, I agree, better to add these deprecated units to the dictionary. I made the changes.
Unfortunately, we can't add "MS" to c_DEPR_ABBREVS because we use that dictionary in the definition of to_offset, and in thus casee"MS" means "MonthBegin". For Timedelta, "ms" means milliseconds, so we want to deprecate "MS" for "Milli" to avoid confusion.
I wonder, can we get these changes into 2.2.x? The reason: we deprecated already uppercase strings “MIN”, “MS”, “US”, and “NS” denoting frequencies for Period and offsets in favor of lowercase strings.
Seems like CI failures aren't related to my changes.
There was a problem hiding this comment.
we want to deprecate "MS" for "Milli" to avoid confusion.
sorry I might have missed something but where was this discussed? I don't remember it now. "month begin" isn't valid for timedelta, so I think there's no risk of confusion here?
I wonder, can we get these changes into 2.2.x?
there's already 2.2.0 and 2.2.1 out, I think it's too late now
There was a problem hiding this comment.
As discussed: by
deprecate "MS" for "Milli"
you meant "deprecate "MS" for milliseconds", not
deprecate "MS" in favour of "Milli"
as I'd interpreted it
All good then 👍
There was a problem hiding this comment.
yeah, my wording wasn't clear enough. I meant: deprecate "MS" for milliseconds".
I updated Timedelta docstring, and added a note to v3.0.0, but I am unsure if we need the note, because using uppercase strings “MIN”, “MS”, “US”, “NS” for units are not documented.
There was a problem hiding this comment.
same as in the other PR, shall we mark this as draft until we've enfored the existing deprecations, so that the codebase can get a little cleaner?
There was a problem hiding this comment.
thanks, sounds reasonable, I marked this PR as draft.
natmokval
added
Deprecate
|
This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this. |
|
closed via #59051 |
Deprecated uppercase strings
“MIN”, “MS”, “US”, “NS”denoting units from the classTimedeltaand the methodto_timedelta.The reason: for period/offsets these strings are already deprecated. Instead of the uppercase strings we use lowercase:
“min”, “ms”, “us”, “ns”.