Skip to content
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

DEPR: deprecate strings T, S, L, U, and N in offsets frequencies, resolution abbreviations, _attrname_to_abbrevs #54061

Conversation

natmokval
Copy link
Contributor

@natmokval natmokval commented Jul 10, 2023

xref #52536

deprecated codes 'T' and 'L' in _attrname_to_abbrevs/_abbrev_to_attrnames, added a test for FutureWarning.

EDIT:

  • Deprecated in Timedelta units 'T', 'L', 'U', 'N' in favour of 'min', 'ms', 'us', 'ns'.
  • Deprecated aliases 'T', 'L', 'U', 'N' for time series frequencies in favour of 'min', 'ms', 'us', 'ns'.
  • Deprecated resolutions 'T', 'L', 'U', 'N' for Timedelta.resolution_string in favour of 'min', 'ms', 'us', 'ns'.

@natmokval
Copy link
Contributor Author

natmokval commented Jul 10, 2023

@jbrockmendel, could you please take a look at my pr? I started deprecating codes “T” and “L” in _attrname_to_abbrevs/_abbrev_to_attrnames and I got an issue. Could you please help me with this?

First, If we deprecate “T” and “L”, we should replace them with new abbreviations. I don't know that we can use instead of “T” and “L”. Can we use one of the abbreviations, which we have for minutes and milliseconds, e.g.: min and ms?

And another question: in class Minute(Tick) and class Milli(Tick) we use “T” and “L” as a _prefix. As I understand we deprecate them as well and we replace them with the new abbreviations for minutes and milliseconds. Am I correct?

@mroeschke mroeschke added the Deprecate Functionality to remove in pandas label Jul 11, 2023
@jbrockmendel
Copy link
Member

First, If we deprecate “T” and “L”, we should replace them with new abbreviations. I don't know that we can use instead of “T” and “L”. Can we use one of the abbreviations, which we have for minutes and milliseconds, e.g.: min and ms?

And another question: in class Minute(Tick) and class Milli(Tick) we use “T” and “L” as a _prefix. As I understand we deprecate them as well and we replace them with the new abbreviations for minutes and milliseconds. Am I correct?

Yes and yes.

@natmokval
Copy link
Contributor Author

Hi @jbrockmendel. I have a question. Should we deprecate 'A' for 'year' as well? We can left 'Y' as an alias for 'year'.

My qustion is related to PR: DEPR offsets: rename ‘M’ to ‘ME’ #52064. I started deprecating for offsets 'A' in favour of 'AE' and I can do either:

  • rename A to AE and Y to YE
  • deprecate A/AE in favour of Y/YE

@MarcoGorelli MarcoGorelli changed the title DEPR: deprecate codes T and L in _attrname_to_abbrevs/_abbrev_to_attrnames DEPR: deprecate codes T, L, N, and U in _attrname_to_abbrevs/_abbrev_to_attrnames Aug 1, 2023
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, getting close! Looks like there's just some doctest failures, and some asv ones

Also this is in the docs build

RuntimeError: Non Expected exception in /home/runner/work/pandas/pandas/doc/source/user_guide/scale.rst line None

so there may be something in that file too

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Have left some comments (which maybe you were going to address anyway 🙈 either way, thought I'd leave them in case)

pandas/_libs/tslibs/dtypes.pyx Outdated Show resolved Hide resolved
pandas/_libs/tslibs/dtypes.pyx Outdated Show resolved Hide resolved
pandas/_libs/tslibs/dtypes.pyx Outdated Show resolved Hide resolved
pandas/core/dtypes/dtypes.py Outdated Show resolved Hide resolved
pandas/_libs/tslibs/dtypes.pyi Outdated Show resolved Hide resolved
pandas/core/tools/timedeltas.py Outdated Show resolved Hide resolved
pandas/tests/indexes/timedeltas/test_timedelta_range.py Outdated Show resolved Hide resolved
pandas/tests/scalar/period/test_asfreq.py Show resolved Hide resolved
pandas/tests/scalar/timedelta/test_timedelta.py Outdated Show resolved Hide resolved
pandas/tests/tseries/frequencies/test_freq_code.py Outdated Show resolved Hide resolved
@natmokval
Copy link
Contributor Author

@MarcoGorelli, @jbrockmendel, could you please take a look at my changes? I corrected the pr as you suggested. Seems like failures in ci aren't related to my changes.

@natmokval natmokval changed the title DEPR: deprecate codes T, S, L, U, and N in _attrname_to_abbrevs/_abbrev_to_attrnames DEPR: deprecate strings T, S, L, U, and N in offsets frequencies, resolution abbreviations, _attrname_to_abbrevs Aug 18, 2023
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally looks good, just got some minor comments

I think we should do hours as well, but please keep that to a separate PR

@@ -905,11 +905,11 @@ into ``freq`` keyword arguments. The available date offsets and associated frequ
:class:`~pandas.tseries.offsets.CustomBusinessHour`, ``'CBH'``, "custom business hour"
:class:`~pandas.tseries.offsets.Day`, ``'D'``, "one absolute day"
:class:`~pandas.tseries.offsets.Hour`, ``'H'``, "one hour"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't need changing in this PR (can be done as a follow-up, there's plenty of time until pandas 2.2), but if we're doing these renamings, then 'H' should probably be renamed too (and CBH, and BH)

I'd stop there - then there a simple rule: anything sub-daily is lowercase, anything daily or higher is upper-case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the comments. I agree, it would be better to rename 'H', 'CBH', and 'BH' too, I'll do it in a separate PR. I like the idea to keep anything sub-daily lowercase, and daily or higher upper-case.

doc/source/whatsnew/v2.2.0.rst Outdated Show resolved Hide resolved
pandas/_libs/tslibs/offsets.pyx Outdated Show resolved Hide resolved
pandas/tests/frame/methods/test_asof.py Outdated Show resolved Hide resolved
@mroeschke mroeschke modified the milestones: 2.1, 2.2 Aug 23, 2023
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good to me, fantastic work here! Sorry it's taken a while to review. I think this should go in

Leaving open a bit in case anyone has objections, but then will merge

@natmokval
Copy link
Contributor Author

Looks really good to me, fantastic work here! Sorry it's taken a while to review. I think this should go in

Leaving open a bit in case anyone has objections, but then will merge

Thank you for helping me with this PR.
Cats are amazing :)

@MarcoGorelli MarcoGorelli added the Frequency DateOffsets label Aug 29, 2023
@MarcoGorelli
Copy link
Member

Right, let's do this, thanks @natmokval

@MarcoGorelli MarcoGorelli merged commit b2dcf2e into pandas-dev:main Aug 29, 2023
mroeschke pushed a commit to mroeschke/pandas that referenced this pull request Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deprecate Functionality to remove in pandas Frequency DateOffsets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants