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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 13 commits into from
Feb 7, 2024
Merged

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
39f59c4
raise ValueError in asfreq for invalid period freq
natmokval Jan 18, 2024
2b602bd
Merge branch 'main' into enh-error-msg-asfreq-invalid-period-freq
natmokval Jan 25, 2024
ff1c6e7
correct PeriodIndex.asfreq, fix test, add tests
natmokval Jan 25, 2024
fff7c9d
move raising ValueError to to_offset
natmokval Feb 1, 2024
a389d11
fix pre-commit errors
natmokval Feb 1, 2024
d2ea914
correct def to_offset, dt64arr_to_periodarr and fix tests
natmokval Feb 2, 2024
3735e23
fix tests
natmokval Feb 2, 2024
b1e6ba6
correct def to_offset, fix tests
natmokval Feb 5, 2024
c41557a
add a note to v2.2.1.rst
natmokval Feb 6, 2024
17fd84c
resolve conflict
natmokval Feb 6, 2024
721637b
Merge remote-tracking branch 'upstream/main' into enh-error-msg-asfre…
MarcoGorelli Feb 7, 2024
deea30d
try simplifying / clarifying
MarcoGorelli Feb 7, 2024
4cbcc5e
revert _get_period_alias simplification
MarcoGorelli Feb 7, 2024
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
8 changes: 7 additions & 1 deletion pandas/_libs/tslibs/offsets.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4846,7 +4846,7 @@ cpdef to_offset(freq, bint is_period=False):
)

elif PyDelta_Check(freq):
return delta_to_tick(freq)
delta = delta_to_tick(freq)
Copy link
Member

Choose a reason for hiding this comment

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

maybe on line 4841, we can do

    if isinstance(freq, BaseOffset):
        delta = freq

so then that gets validated too?

Copy link
Member

Choose a reason for hiding this comment

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

is it possible to do this assignment on line 4841 as well? just in case an offset which isn't valid for periods is passed

Copy link
Contributor Author

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. I tried to do this assignment on line 4841

if isinstance(freq, BaseOffset):
    delta = freq

but then I got 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.
Which is why 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")

Copy link
Member

Choose a reason for hiding this comment

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

I think you can just move this down to before elif PyDelta_Check(freq):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, I saw you already made commit for it


elif isinstance(freq, str):
delta = None
Expand Down Expand Up @@ -4964,6 +4964,12 @@ cpdef to_offset(freq, bint is_period=False):
if delta is None:
raise ValueError(INVALID_FREQ_ERR_MSG.format(freq))

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

return delta


Expand Down
7 changes: 1 addition & 6 deletions pandas/core/arrays/period.py
View file
Open in desktop
Original file line number Diff line number Diff line change
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
10 changes: 5 additions & 5 deletions pandas/tests/dtypes/test_dtypes.py
View file
Open in desktop
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(TypeError, 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
View file
Open in desktop
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()
34 changes: 33 additions & 1 deletion pandas/tests/indexes/period/methods/test_asfreq.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ def test_asfreq(self):

msg = "How must be one of S or E"
with pytest.raises(ValueError, match=msg):
pi7.asfreq("T", "foo")
pi7.asfreq("min", "foo")
result1 = pi1.asfreq("3M")
result2 = pi1.asfreq("M")
expected = period_range(freq="M", start="2001-12", end="2001-12")
Expand Down Expand Up @@ -136,3 +136,35 @@ 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)
4 changes: 2 additions & 2 deletions pandas/tests/resample/test_period_index.py
View file
Open in desktop
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
View file
Open in desktop
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")
11 changes: 6 additions & 5 deletions pandas/tests/scalar/period/test_period.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,20 +41,21 @@ 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):
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")
msg = "CustomBusinessDay is not supported as period frequency"
with pytest.raises(TypeError, 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 •
edited
Loading

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