Skip to content

CLN: replace unnecessary freq_to_period_freqstr with PeriodDtype constructor #56550

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

Merged
merged 18 commits into from
Feb 28, 2024
Merged
Changes from 4 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions pandas/core/generic.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@
Timestamp,
to_offset,
)
from pandas._libs.tslibs.dtypes import freq_to_period_freqstr
from pandas._typing import (
AlignJoin,
AnyArrayLike,
Expand Down Expand Up @@ -135,6 +134,7 @@
from pandas.core.dtypes.dtypes import (
DatetimeTZDtype,
ExtensionDtype,
PeriodDtype,
)
from pandas.core.dtypes.generic import (
ABCDataFrame,
Expand Down Expand Up @@ -11274,9 +11274,9 @@ def _shift_with_freq(self, periods: int, axis: int, freq) -> Self:
if freq != orig_freq:
assert orig_freq is not None # for mypy
raise ValueError(
f"Given freq {freq_to_period_freqstr(freq.n, freq.name)} "
f"Given freq {PeriodDtype(freq)._freqstr} "
Copy link
Member

Choose a reason for hiding this comment

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

does this change the message? if so, could you show before vs after please?

if not, then is it possible to do this replacement across the whole codebase and get rid of freq_to_period_freqstr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

does this change the message? if so, could you show before vs after please?

It doesn't change the message.

if not, then is it possible to do this replacement across the whole codebase and get rid of freq_to_period_freqstr?

I replaced freq_to_period_freqstr with PeriodDtype(offset)._freqstr where it works. Now I am trying to do this replacement in pandas/tseries/frequencies.py and in pandas/_libs/tslibs/period.pyx, but I get failures. I am not sure we can complitely rid of freq_to_period_freqstr

f"does not match PeriodIndex freq "
f"{freq_to_period_freqstr(orig_freq.n, orig_freq.name)}"
f"{PeriodDtype(orig_freq)._freqstr}"
)
new_ax = index.shift(periods)
else:
Expand Down