CLN: replace unnecessary freq_to_period_freqstr with PeriodDtype constructor
#56550
Conversation
…frequencies.py, period.pyx
| f"Given freq {freq_to_period_freqstr(freq.n, freq.name)} " | ||
| f"Given freq {PeriodDtype(freq)._freqstr} " |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
MarcoGorelli
changed the title
freq_to_period_freqstr with PeriodDtype constructor
|
I replaced One comment: because otherwise the @MarcoGorelli, could you please take a look at this PR? |
pandas/core/arrays/period.py
Outdated
| with warnings.catch_warnings(): | ||
| warnings.simplefilter("ignore") |
There was a problem hiding this comment.
looks a bit dangerous :) is this for the business day warning? if so, I'd suggest either enforcing that one first, or using warnings.filterwarnings and only filtering the warning you mean to
There was a problem hiding this comment.
yes, this is for the business day warning. I agree, it's better to use warnings.filterwarnings instead of warnings.simplefilter. I made changes in order to filter: "Future warning "PeriodDtype[B] is deprecated"
There was a problem hiding this comment.
we can rid of this warnings.filterwarnings after enforcing depreciation of PeriodDtype[B], or we can postpone merging this PR until enforcing deprecation of PeriodDtype[B] is done.
| freqstr = freq_to_period_freqstr(self.freq.n, self.freq.name) | ||
| other_freqstr = freq_to_period_freqstr(other.n, other.name) | ||
| freqstr = PeriodDtypeBase( | ||
| self.freq._period_dtype_code, self.freq.n | ||
| )._freqstr | ||
| if hasattr(other, "_period_dtype_code"): | ||
| other_freqstr = PeriodDtypeBase( | ||
| other._period_dtype_code, other.n | ||
| )._freqstr | ||
| else: | ||
| other_freqstr = other.freqstr |
There was a problem hiding this comment.
why do we need the if-else branch for other but not for self.freq?
There was a problem hiding this comment.
self.freq always has attribute "_period_dtype_code", bur other sometimes doesn't.
For example in many tests in pandas/tests/arithmetic/test_period.py( e.g. test_pi_add_sub_timedeltalike_freq_mismatch_monthly) other is equal pd.offsets.YearBegin(2)
| freq = freq_to_period_freqstr(1, df.index.freq.rule_code) | ||
| freq = re.split(r"\d", PeriodDtype(df.index.freq)._freqstr)[-1] |
There was a problem hiding this comment.
why does this one require regex splitting, but others don't?
There was a problem hiding this comment.
Hmm, seems like we don't need this regex splitting here and can replace freq = re.split(r"\d", PeriodDtype(df.index.freq)._freqstr)[-1] with freq = PeriodDtype(df.index.freq)._freqstr
I will make this change.
There is another test test_line_plot_period_mlt_frame in which we use the same regex splitting. I tried to do the similar replacement (with freq = PeriodDtype(df.index.freq)._freqstr[1:]), but the test failed for "1s".
There was a problem hiding this comment.
I think in test_line_plot_period_mlt_frame we can change freq from
freq = re.split(r"\d", PeriodDtype(df.index.freq)._freqstr)[-1]
freq = df.index.asfreq(freq).freq
to
freq = df.index.freq.rule_code
and then get rid of regex splitting. I modified the test.
There was a problem hiding this comment.
if we use the same replacement in test_line_plot_datetime_frame, e.g. replace
freq = PeriodDtype(df.index.freq)._freqstr
freq = df.index.to_period(freq).freq
with
freq = df.index.freq.rule_code
the test passes. Should we modify this test as well?
There was a problem hiding this comment.
Nice!
@jbrockmendel I think you'd suggested this clean up, fancy taking a quick look?
|
Thanks @natmokval |
…constructor (pandas-dev#56550) * replace freq_to_period_freqstr() with PeriodDtype(freq)._freqstr in _shift_with_freq() * replace freq_to_period_freqstr with PeriodDtype()._freqstr except in frequencies.py, period.pyx * fix pre-commit errors * remove freq_to_period_freqstr from _maybe_coerce_freq and from tests * remove freq_to_period_freqstr from period.pyx * correct raise_on_incompatible * correct raise_on_incompatible again * replace warnings.simplefilter with warnings.filterwarnings in raise_on_incompatible * remove regex splitting from test_line_plot_datetime_frame * remove regex splitting from test_line_plot_period_mlt_frame
xref #52064
replaced
freq_to_period_freqstr()withPeriodDtype(freq)._freqstrin ValueError message in the definition of_shift_with_freq()