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: 'A' for yearly frequency and YearEnd in favour of 'Y' #55252

Merged
merged 15 commits into from Oct 6, 2023

Conversation

natmokval
Copy link
Contributor

@natmokval natmokval commented Sep 22, 2023

xref #54275, #54061

deprecated string 'A' for yearly frequency and YearEnd in favour of 'Y'.

EDIT:
deprecated annual frequencies with various fiscal year ends: "A-DEC", "A-JAN", etc. in favour of "Y-DEC", "Y-JAN", etc.

@natmokval natmokval marked this pull request as ready for review September 25, 2023 16:00
@mroeschke mroeschke added Frequency DateOffsets Deprecate Functionality to remove in pandas labels Sep 25, 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.

in offsets.pyx I think you'll need to generalise this part

                if is_period is False and name == "M":
                    warnings.warn(
                        "\'M\' will be deprecated, please use \'ME\' "
                        "for \'month end\'",
                        UserWarning,
                        stacklevel=find_stack_level(),
                    )
                    name = "ME"
                if is_period is True and name == "ME":
                    raise ValueError(
                        r"for Period, please use \'M\' "
                        "instead of \'ME\'"
                    )
                elif is_period is True and name == "M":
                    name = "ME"

@natmokval
Copy link
Contributor Author

in offsets.pyx I think you'll need to generalise this part

                if is_period is False and name == "M":
                    warnings.warn(
                        "\'M\' will be deprecated, please use \'ME\' "
                        "for \'month end\'",
                        UserWarning,
                        stacklevel=find_stack_level(),
                    )
                    name = "ME"
                if is_period is True and name == "ME":
                    raise ValueError(
                        r"for Period, please use \'M\' "
                        "instead of \'ME\'"
                    )
                elif is_period is True and name == "M":
                    name = "ME"

Thank you for the comment. I generalised it by adding a dictionary with frequencies deprecated for offsets.

@natmokval
Copy link
Contributor Author

I deprecated annual frequencies with various fiscal year ends: "A-DEC", "A-JAN", etc. in favour of "Y-DEC", "Y-JAN", etc., added tests for FutureWarning and notes in v2.2.0. @MarcoGorelli, could you please take a look at this PR?

doc/source/user_guide/timeseries.rst Outdated Show resolved Hide resolved
pandas/_libs/tslibs/dtypes.pyx Outdated Show resolved Hide resolved
pandas/_libs/tslibs/offsets.pyx Outdated Show resolved Hide resolved
Comment on lines 4642 to 4650
if prefix in c_DEPR_ABBREVS:
warnings.warn(
f"\'{prefix}\' is deprecated and will be removed in a "
f"future version. Please use \'{c_DEPR_ABBREVS.get(prefix)}\' "
f"instead of \'{prefix}\'.",
FutureWarning,
stacklevel=find_stack_level(),
)
prefix = c_DEPR_ABBREVS[prefix]
Copy link
Member

Choose a reason for hiding this comment

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

same as in the other PR, it's not clear to me why this moved down - could you explain please?

Copy link
Contributor Author

@natmokval natmokval Oct 3, 2023

Choose a reason for hiding this comment

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

because I wanted to keep this together with line 4668: offset = _get_offset(prefix) where we use this prefix. There is no need to do it, I can move it back.

Copy link
Member

Choose a reason for hiding this comment

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

yeah that might be better - wouldn't we risk missing the if prefix in {"D", "H", "min", "s", "ms", "us", "ns"}: like this?

Copy link
Contributor Author

@natmokval natmokval Oct 3, 2023

Choose a reason for hiding this comment

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

hmm, I agree, we might have problems if frequency is 'T', 'S', 'L', 'U', or 'N'. Maybe we should check
if prefix in c_DEPR_ABBREVS: before we are doing if prefix in {"D", "H", "min", "s", "ms", "us", "ns"}:
I will update my PR

Copy link
Contributor Author

@natmokval natmokval Oct 4, 2023

Choose a reason for hiding this comment

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

I moved the check if prefix in c_DEPR_ABBREVS: back to line 4642 and added a test that fails if the order of the checks isn't correct.

Copy link
Member

Choose a reason for hiding this comment

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

added a test that fails if the order of the checks isn't correct.

this is music to my ears - well done!

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.

solid work! let's get this in and move on to the next one 💪

@MarcoGorelli MarcoGorelli added this to the 2.2 milestone Oct 6, 2023
@MarcoGorelli MarcoGorelli merged commit 7ec95e4 into pandas-dev:main Oct 6, 2023
39 checks passed
@natmokval
Copy link
Contributor Author

Thank you for helping me with this PR.

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.

None yet

3 participants