Skip to content

ENH: Raise TypeError for offsets which are not supported as period frequency #55844

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

Merged
merged 8 commits into from
Nov 7, 2023
Merged

ENH: Raise TypeError for offsets which are not supported as period frequency #55844

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
df8acd0
raise ValueError for offsets which are not supported as period frequency
natmokval Nov 6, 2023
0de52d6
correct error message in test_asfreq_MS
natmokval Nov 6, 2023
ccee38d
simplify the condition for ValueError
natmokval Nov 6, 2023
d75b773
simlify the condition, correct tests
natmokval Nov 7, 2023
e88c8d8
change the condition and Error class in Period constructor
natmokval Nov 7, 2023
7bb1457
Merge branch 'main' into bug-QuarterBegin-doesnt-work-with-Period
natmokval Nov 7, 2023
e436a92
add a note to v2.2.0
natmokval Nov 7, 2023
8fe159c
change note in v2.2.0
natmokval Nov 7, 2023
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
27 changes: 27 additions & 0 deletions pandas/_libs/tslibs/period.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,12 +117,34 @@ from pandas._libs.tslibs.offsets cimport (
from pandas._libs.tslibs.offsets import (
INVALID_FREQ_ERR_MSG,
BDay,
BQuarterBegin,
BQuarterEnd,
BusinessMonthBegin,
BusinessMonthEnd,
BYearBegin,
BYearEnd,
MonthBegin,
QuarterBegin,
YearBegin,
)

cdef:
enum:
INT32_MIN = -2_147_483_648LL

# following offsets classes are not supported as period frequencies
PERIOD_DEPR_OFFSETS = {
YearBegin,
BYearBegin,
BYearEnd,
QuarterBegin,
BQuarterBegin,
BQuarterEnd,
MonthBegin,
BusinessMonthBegin,
BusinessMonthEnd,
}


ctypedef struct asfreq_info:
int64_t intraday_conversion_factor
Expand Down Expand Up @@ -2728,6 +2750,11 @@ class Period(_Period):
freq = cls._maybe_convert_freq(freq)
nanosecond = 0

if freq.__class__ in PERIOD_DEPR_OFFSETS:
raise ValueError(
f"{(type(freq).__name__)} 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.

instead of hard-coding the offsets and checking this, could we just do

if not hasattr(freq, '_period_dtype_code'):
    raise ValueError(...)

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I corrected the condition and the error message. We have _period_dtype_code in class CustomBusinessDay (see link) that is why I excluded CustomBusinessDay.

I thought, isn't better to use TypeError instead of ValueError?
Should we add a note to v2.2.0? Something like Bug in incorrectly allowing construction of :class:'Period' ...

Copy link
Member

@MarcoGorelli MarcoGorelli Nov 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

thanks - yeah TypeError is probably better

and yes, a 2.2.0 whatsnew note would be good

maybe we can just do

try:
    period_dtype_code = freq._period_dtype_code
except (AttributeError, TypeError):
    # AttributeError: _period_dtype_code might not exist
    # TypeError: _period_dtype_code might intentionally raise
    raise TypeError(f"{(type(freq).__name__)} is not supported as period frequency"

and then reuse period_dtype_code further down where there's freq._period_dtype_code?

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 did as you suggested. It works very well.


if ordinal is not None and value is not None:
raise ValueError("Only value or ordinal but not both should be "
"given but not both")
Expand Down
15 changes: 15 additions & 0 deletions pandas/tests/indexes/period/test_constructors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,6 +553,21 @@ def test_map_with_string_constructor(self):
# lastly, values should compare equal
tm.assert_index_equal(res, expected)

@pytest.mark.parametrize(
"freq",
[
offsets.BYearBegin(),
offsets.YearBegin(2),
offsets.QuarterBegin(startingMonth=12),
offsets.BusinessMonthEnd(2),
],
)
def test_offsets_not_supported(self, freq):
# GH#55785
msg = f"{type(freq).__name__} is not supported as period frequency"
with pytest.raises(ValueError, match=msg):
Period(year=2014, freq=freq)


class TestShallowCopy:
def test_shallow_copy_empty(self):
Expand Down
1 change: 1 addition & 0 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 @@ -825,6 +825,7 @@ def test_asfreq_MS(self):
with pytest.raises(ValueError, match=msg):
initial.asfreq(freq="MS", how="S")

msg = "MonthBegin is not supported as period frequency"
with pytest.raises(ValueError, match=msg):
Period("2013-01", "MS")

Expand Down