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

ENH: raise ValueError if invalid period freq pass to asfreq when the index of df is a PeriodIndex #56945

Merged
1 change: 1 addition & 0 deletions doc/source/whatsnew/v2.2.1.rst
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ Bug fixes
- Fixed bug in :func:`pandas.api.interchange.from_dataframe` which was raising for empty inputs (:issue:`56700`)
- Fixed bug in :func:`pandas.api.interchange.from_dataframe` which wasn't converting columns names to strings (:issue:`55069`)
- Fixed bug in :meth:`DataFrame.__getitem__` for empty :class:`DataFrame` with Copy-on-Write enabled (:issue:`57130`)
- Fixed bug in :meth:`PeriodIndex.asfreq` which was silently converting frequencies which are not supported as period frequencies instead of raising an error (:issue:`56945`)

.. ---------------------------------------------------------------------------
.. _whatsnew_221.other:
Expand Down
32 changes: 18 additions & 14 deletions pandas/_libs/tslibs/offsets.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -4271,9 +4271,7 @@ cdef class CustomBusinessDay(BusinessDay):
@property
def _period_dtype_code(self):
# GH#52534
raise TypeError(
"CustomBusinessDay is not supported as period frequency"
)
raise ValueError(f"{self.base} is not supported as period frequency")

_apply_array = BaseOffset._apply_array

Expand Down Expand Up @@ -4837,19 +4835,19 @@ cpdef to_offset(freq, bint is_period=False):
if freq is None:
return None

if isinstance(freq, BaseOffset):
return freq

if isinstance(freq, tuple):
raise TypeError(
f"to_offset does not support tuples {freq}, pass as a string instead"
)

if isinstance(freq, BaseOffset):
result = freq

elif PyDelta_Check(freq):
return delta_to_tick(freq)
result = delta_to_tick(freq)

elif isinstance(freq, str):
delta = None
result = None
stride_sign = None

try:
Expand Down Expand Up @@ -4950,21 +4948,27 @@ cpdef to_offset(freq, bint is_period=False):
offset = _get_offset(prefix)
offset = offset * int(np.fabs(stride) * stride_sign)

if delta is None:
delta = offset
if result is None:
result = offset
else:
delta = delta + offset
result = result + offset
except (ValueError, TypeError) as err:
raise ValueError(INVALID_FREQ_ERR_MSG.format(
f"{freq}, failed to parse with error message: {repr(err)}")
)
else:
delta = None
result = None

if delta is None:
if result is None:
raise ValueError(INVALID_FREQ_ERR_MSG.format(freq))

return delta
if is_period and not hasattr(result, "_period_dtype_code"):
if isinstance(freq, str):
raise ValueError(f"{result.name} is not supported as period frequency")
else:
raise ValueError(f"{freq} is not supported as period frequency")

return result


# ----------------------------------------------------------------------
Expand Down
11 changes: 3 additions & 8 deletions pandas/core/arrays/period.py
Original file line number Diff line number Diff line change
Expand Up @@ -733,8 +733,8 @@ def asfreq(self, freq=None, how: str = "E") -> Self:
'2015-01'], dtype='period[M]')
"""
how = libperiod.validate_end_alias(how)
if isinstance(freq, BaseOffset):
freq = freq_to_period_freqstr(freq.n, freq.name)
if isinstance(freq, BaseOffset) and hasattr(freq, "_period_dtype_code"):
freq = PeriodDtype(freq)._freqstr
freq = Period._maybe_convert_freq(freq)

base1 = self._dtype._dtype_code
Expand Down Expand Up @@ -1186,12 +1186,7 @@ def dt64arr_to_periodarr(

reso = get_unit_from_dtype(data.dtype)
freq = Period._maybe_convert_freq(freq)
try:
base = freq._period_dtype_code
except (AttributeError, TypeError) as err:
# AttributeError: _period_dtype_code might not exist
# TypeError: _period_dtype_code might intentionally raise
raise TypeError(f"{freq.name} is not supported as period frequency") from err
base = freq._period_dtype_code
return c_dt64arr_to_periodarr(data.view("i8"), base, tz, reso=reso), freq


Expand Down
5 changes: 4 additions & 1 deletion pandas/plotting/_matplotlib/timeseries.py
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,10 @@ def _get_ax_freq(ax: Axes):


def _get_period_alias(freq: timedelta | BaseOffset | str) -> str | None:
freqstr = to_offset(freq, is_period=True).rule_code
if isinstance(freq, BaseOffset):
freqstr = freq.name
else:
freqstr = to_offset(freq, is_period=True).rule_code
MarcoGorelli marked this conversation as resolved.
Show resolved Hide resolved

return get_period_alias(freqstr)

Expand Down
10 changes: 5 additions & 5 deletions pandas/tests/dtypes/test_dtypes.py
Original file line number Diff line number Diff line change
Expand Up @@ -445,12 +445,12 @@ def test_construction(self):

def test_cannot_use_custom_businessday(self):
# GH#52534
msg = "CustomBusinessDay is not supported as period frequency"
msg = "C is not supported as period frequency"
msg1 = "<CustomBusinessDay> is not supported as period frequency"
msg2 = r"PeriodDtype\[B\] is deprecated"
with pytest.raises(TypeError, match=msg):
with tm.assert_produces_warning(FutureWarning, match=msg2):
PeriodDtype("C")
with pytest.raises(TypeError, match=msg):
with pytest.raises(ValueError, match=msg):
PeriodDtype("C")
with pytest.raises(ValueError, match=msg1):
with tm.assert_produces_warning(FutureWarning, match=msg2):
PeriodDtype(pd.offsets.CustomBusinessDay())

Expand Down
2 changes: 1 addition & 1 deletion pandas/tests/indexes/datetimes/methods/test_to_period.py
Original file line number Diff line number Diff line change
Expand Up @@ -221,5 +221,5 @@ def test_to_period_offsets_not_supported(self, freq):
# GH#56243
msg = f"{freq[1:]} is not supported as period frequency"
ts = date_range("1/1/2012", periods=4, freq=freq)
with pytest.raises(TypeError, match=msg):
with pytest.raises(ValueError, match=msg):
ts.to_period()
51 changes: 51 additions & 0 deletions pandas/tests/indexes/period/methods/test_asfreq.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import re

import pytest

from pandas import (
Expand All @@ -7,6 +9,8 @@
)
import pandas._testing as tm

from pandas.tseries import offsets


class TestPeriodIndex:
def test_asfreq(self):
Expand Down Expand Up @@ -136,3 +140,50 @@ def test_asfreq_with_different_n(self):

excepted = Series([1, 2], index=PeriodIndex(["2020-02", "2020-04"], freq="M"))
tm.assert_series_equal(result, excepted)

@pytest.mark.parametrize(
"freq",
[
"2BMS",
"2YS-MAR",
"2bh",
],
)
def test_pi_asfreq_not_supported_frequency(self, freq):
# GH#55785
msg = f"{freq[1:]} is not supported as period frequency"

pi = PeriodIndex(["2020-01-01", "2021-01-01"], freq="M")
with pytest.raises(ValueError, match=msg):
pi.asfreq(freq=freq)

@pytest.mark.parametrize(
"freq",
[
"2BME",
"2YE-MAR",
"2QE",
],
)
def test_pi_asfreq_invalid_frequency(self, freq):
# GH#55785
msg = f"Invalid frequency: {freq}"

pi = PeriodIndex(["2020-01-01", "2021-01-01"], freq="M")
with pytest.raises(ValueError, match=msg):
pi.asfreq(freq=freq)

@pytest.mark.parametrize(
"freq",
[
offsets.MonthBegin(2),
offsets.BusinessMonthEnd(2),
],
)
def test_pi_asfreq_invalid_baseoffset(self, freq):
# GH#56945
msg = re.escape(f"{freq} is not supported as period frequency")

pi = PeriodIndex(["2020-01-01", "2021-01-01"], freq="M")
with pytest.raises(ValueError, match=msg):
pi.asfreq(freq=freq)
4 changes: 2 additions & 2 deletions pandas/tests/resample/test_period_index.py
Original file line number Diff line number Diff line change
Expand Up @@ -1032,8 +1032,8 @@ def test_resample_lowercase_frequency_deprecated(
offsets.BusinessHour(2),
],
)
def test_asfreq_invalid_period_freq(self, offset, frame_or_series):
# GH#9586
def test_asfreq_invalid_period_offset(self, offset, frame_or_series):
# GH#55785
msg = f"Invalid offset: '{offset.base}' for converting time series "

obj = frame_or_series(range(5), index=period_range("2020-01-01", periods=5))
Expand Down
5 changes: 2 additions & 3 deletions pandas/tests/scalar/period/test_asfreq.py
Original file line number Diff line number Diff line change
Expand Up @@ -820,10 +820,9 @@ def test_asfreq_MS(self):

assert initial.asfreq(freq="M", how="S") == Period("2013-01", "M")

msg = INVALID_FREQ_ERR_MSG
msg = "MS is not supported as period frequency"
with pytest.raises(ValueError, match=msg):
initial.asfreq(freq="MS", how="S")

msg = "MonthBegin is not supported as period frequency"
with pytest.raises(TypeError, match=msg):
with pytest.raises(ValueError, match=msg):
Period("2013-01", "MS")
16 changes: 9 additions & 7 deletions pandas/tests/scalar/period/test_period.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
datetime,
timedelta,
)
import re

import numpy as np
import pytest
Expand Down Expand Up @@ -40,21 +41,22 @@ class TestPeriodDisallowedFreqs:
)
def test_offsets_not_supported(self, freq, freq_msg):
# GH#55785
msg = f"{freq_msg} is not supported as period frequency"
with pytest.raises(TypeError, match=msg):
msg = re.escape(f"{freq} is not supported as period frequency")
with pytest.raises(ValueError, match=msg):
Period(year=2014, freq=freq)

def test_custom_business_day_freq_raises(self):
# GH#52534
msg = "CustomBusinessDay is not supported as period frequency"
with pytest.raises(TypeError, match=msg):
msg = "C is not supported as period frequency"
with pytest.raises(ValueError, match=msg):
Period("2023-04-10", freq="C")
with pytest.raises(TypeError, match=msg):
msg = f"{offsets.CustomBusinessDay().base} is not supported as period frequency"
with pytest.raises(ValueError, match=msg):
Period("2023-04-10", freq=offsets.CustomBusinessDay())

def test_invalid_frequency_error_message(self):
msg = "WeekOfMonth is not supported as period frequency"
with pytest.raises(TypeError, match=msg):
msg = "WOM-1MON is not supported as period frequency"
with pytest.raises(ValueError, match=msg):
Period("2012-01-02", freq="WOM-1MON")
Comment on lines -56 to 60
Copy link
Member

Choose a reason for hiding this comment

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

nice! yeah better to match what the user passed if possible, good one

Copy link
Contributor Author

@natmokval natmokval Feb 6, 2024

Choose a reason for hiding this comment

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

I am unsure if it's possible to do this assignment on line 4841, if I do it

if isinstance(freq, BaseOffset):
    delta = freq

I get failures. The reason: if we replace return freq with the delta = freq, we go to the line 4962 and assign delta to None and then on line 4965 we raise a ValueError.

Instead of the assignment delta = freq I added the check

if is_period and not hasattr(freq, "_period_dtype_code"):
    raise ValueError(f"{freq.base} is not supported as period frequency")


def test_invalid_frequency_period_error_message(self):
Expand Down
Loading