From df8acd00ea3c8a9fb24a64d40b8b34122859624b Mon Sep 17 00:00:00 2001 From: Natalia Mokeeva Date: Mon, 6 Nov 2023 16:13:36 +0100 Subject: [PATCH 1/7] raise ValueError for offsets which are not supported as period frequency --- pandas/_libs/tslibs/period.pyx | 28 +++++++++++++++++++ .../tests/indexes/period/test_constructors.py | 15 ++++++++++ 2 files changed, 43 insertions(+) diff --git a/pandas/_libs/tslibs/period.pyx b/pandas/_libs/tslibs/period.pyx index 700f500c1d8b8..90f9621d9db28 100644 --- a/pandas/_libs/tslibs/period.pyx +++ b/pandas/_libs/tslibs/period.pyx @@ -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 @@ -2728,6 +2750,12 @@ class Period(_Period): freq = cls._maybe_convert_freq(freq) nanosecond = 0 + for depr_offsets in PERIOD_DEPR_OFFSETS: + if isinstance(freq, depr_offsets): + raise ValueError( + f"{(type(freq).__name__)} is not supported as period frequency" + ) + 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") diff --git a/pandas/tests/indexes/period/test_constructors.py b/pandas/tests/indexes/period/test_constructors.py index f1db5ab28be30..031c522c0979f 100644 --- a/pandas/tests/indexes/period/test_constructors.py +++ b/pandas/tests/indexes/period/test_constructors.py @@ -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): From 0de52d66725f819f61368e01d20b3e37061803c2 Mon Sep 17 00:00:00 2001 From: Natalia Mokeeva Date: Mon, 6 Nov 2023 17:31:35 +0100 Subject: [PATCH 2/7] correct error message in test_asfreq_MS --- pandas/tests/scalar/period/test_asfreq.py | 1 + 1 file changed, 1 insertion(+) diff --git a/pandas/tests/scalar/period/test_asfreq.py b/pandas/tests/scalar/period/test_asfreq.py index 597282e10052e..0c491786370a2 100644 --- a/pandas/tests/scalar/period/test_asfreq.py +++ b/pandas/tests/scalar/period/test_asfreq.py @@ -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") From ccee38dcc984faf37df165fa6386b067550fd9ad Mon Sep 17 00:00:00 2001 From: Natalia Mokeeva Date: Mon, 6 Nov 2023 18:21:45 +0100 Subject: [PATCH 3/7] simplify the condition for ValueError --- pandas/_libs/tslibs/period.pyx | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/pandas/_libs/tslibs/period.pyx b/pandas/_libs/tslibs/period.pyx index 90f9621d9db28..5c7e3c5c4f920 100644 --- a/pandas/_libs/tslibs/period.pyx +++ b/pandas/_libs/tslibs/period.pyx @@ -2750,11 +2750,10 @@ class Period(_Period): freq = cls._maybe_convert_freq(freq) nanosecond = 0 - for depr_offsets in PERIOD_DEPR_OFFSETS: - if isinstance(freq, depr_offsets): - raise ValueError( - f"{(type(freq).__name__)} is not supported as period frequency" - ) + if freq.__class__ in PERIOD_DEPR_OFFSETS: + raise ValueError( + f"{(type(freq).__name__)} is not supported as period frequency" + ) if ordinal is not None and value is not None: raise ValueError("Only value or ordinal but not both should be " From d75b773ad2844b8d1f9efdf737f6ed7e4b4818e8 Mon Sep 17 00:00:00 2001 From: Natalia Mokeeva Date: Tue, 7 Nov 2023 09:43:16 +0100 Subject: [PATCH 4/7] simlify the condition, correct tests --- pandas/_libs/tslibs/period.pyx | 33 ++++--------------- .../tests/indexes/period/test_constructors.py | 17 ++++++---- pandas/tests/scalar/period/test_asfreq.py | 2 +- pandas/tests/scalar/period/test_period.py | 2 +- 4 files changed, 18 insertions(+), 36 deletions(-) diff --git a/pandas/_libs/tslibs/period.pyx b/pandas/_libs/tslibs/period.pyx index 5c7e3c5c4f920..15b9c02291c67 100644 --- a/pandas/_libs/tslibs/period.pyx +++ b/pandas/_libs/tslibs/period.pyx @@ -117,34 +117,13 @@ 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, + CustomBusinessDay, ) 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 @@ -2748,13 +2727,13 @@ class Period(_Period): if freq is not None: freq = cls._maybe_convert_freq(freq) + if (not isinstance(freq, CustomBusinessDay) and + not hasattr(freq, "_period_dtype_code")): + raise ValueError( + f"{freq} is not supported as period frequency" + ) nanosecond = 0 - if freq.__class__ in PERIOD_DEPR_OFFSETS: - raise ValueError( - f"{(type(freq).__name__)} is not supported as period frequency" - ) - 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") diff --git a/pandas/tests/indexes/period/test_constructors.py b/pandas/tests/indexes/period/test_constructors.py index 031c522c0979f..190b64ef02d23 100644 --- a/pandas/tests/indexes/period/test_constructors.py +++ b/pandas/tests/indexes/period/test_constructors.py @@ -554,17 +554,20 @@ def test_map_with_string_constructor(self): tm.assert_index_equal(res, expected) @pytest.mark.parametrize( - "freq", + "freq, freq_msg", [ - offsets.BYearBegin(), - offsets.YearBegin(2), - offsets.QuarterBegin(startingMonth=12), - offsets.BusinessMonthEnd(2), + (offsets.BYearBegin(), ""), + (offsets.YearBegin(2), r"<2 \* YearBegins: month=1>"), + ( + offsets.QuarterBegin(startingMonth=12), + "", + ), + (offsets.BusinessMonthEnd(2), r"<2 \* BusinessMonthEnds>"), ], ) - def test_offsets_not_supported(self, freq): + def test_offsets_not_supported(self, freq, freq_msg): # GH#55785 - msg = f"{type(freq).__name__} is not supported as period frequency" + msg = f"{freq_msg} is not supported as period frequency" with pytest.raises(ValueError, match=msg): Period(year=2014, freq=freq) diff --git a/pandas/tests/scalar/period/test_asfreq.py b/pandas/tests/scalar/period/test_asfreq.py index 0c491786370a2..f5b17eeece000 100644 --- a/pandas/tests/scalar/period/test_asfreq.py +++ b/pandas/tests/scalar/period/test_asfreq.py @@ -825,7 +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" + msg = " is not supported as period frequency" with pytest.raises(ValueError, match=msg): Period("2013-01", "MS") diff --git a/pandas/tests/scalar/period/test_period.py b/pandas/tests/scalar/period/test_period.py index 8cc3ace52a4d4..0484a14567d28 100644 --- a/pandas/tests/scalar/period/test_period.py +++ b/pandas/tests/scalar/period/test_period.py @@ -1628,7 +1628,7 @@ def test_negone_ordinals(): def test_invalid_frequency_error_message(): - msg = "Invalid frequency: " + msg = " is not supported as period frequency" with pytest.raises(ValueError, match=msg): Period("2012-01-02", freq="WOM-1MON") From e88c8d8ba29b8d607aa9a26a97e75760374641d9 Mon Sep 17 00:00:00 2001 From: Natalia Mokeeva Date: Tue, 7 Nov 2023 13:57:17 +0100 Subject: [PATCH 5/7] change the condition and Error class in Period constructor --- pandas/_libs/tslibs/offsets.pyx | 2 +- pandas/_libs/tslibs/period.pyx | 13 ++++++++----- pandas/tests/dtypes/test_dtypes.py | 2 +- pandas/tests/indexes/period/test_constructors.py | 13 +++++-------- pandas/tests/scalar/period/test_asfreq.py | 4 ++-- pandas/tests/scalar/period/test_period.py | 6 +++--- 6 files changed, 20 insertions(+), 20 deletions(-) diff --git a/pandas/_libs/tslibs/offsets.pyx b/pandas/_libs/tslibs/offsets.pyx index df3a2e3ecde48..0150aeadbd0ab 100644 --- a/pandas/_libs/tslibs/offsets.pyx +++ b/pandas/_libs/tslibs/offsets.pyx @@ -4177,7 +4177,7 @@ cdef class CustomBusinessDay(BusinessDay): def _period_dtype_code(self): # GH#52534 raise TypeError( - "CustomBusinessDay cannot be used with Period or PeriodDtype" + "CustomBusinessDay is not supported as period frequency" ) _apply_array = BaseOffset._apply_array diff --git a/pandas/_libs/tslibs/period.pyx b/pandas/_libs/tslibs/period.pyx index 15b9c02291c67..6fcfcdf5e70e4 100644 --- a/pandas/_libs/tslibs/period.pyx +++ b/pandas/_libs/tslibs/period.pyx @@ -2727,10 +2727,13 @@ class Period(_Period): if freq is not None: freq = cls._maybe_convert_freq(freq) - if (not isinstance(freq, CustomBusinessDay) and - not hasattr(freq, "_period_dtype_code")): - raise ValueError( - f"{freq} is not supported as period frequency" + 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" ) nanosecond = 0 @@ -2764,7 +2767,7 @@ class Period(_Period): elif is_period_object(value): other = value - if freq is None or freq._period_dtype_code == other._dtype._dtype_code: + if freq is None or period_dtype_code == other._dtype._dtype_code: ordinal = other.ordinal freq = other.freq else: diff --git a/pandas/tests/dtypes/test_dtypes.py b/pandas/tests/dtypes/test_dtypes.py index 27994708d2bdb..04b3feabb9854 100644 --- a/pandas/tests/dtypes/test_dtypes.py +++ b/pandas/tests/dtypes/test_dtypes.py @@ -444,7 +444,7 @@ def test_construction(self): def test_cannot_use_custom_businessday(self): # GH#52534 - msg = "CustomBusinessDay cannot be used with Period or PeriodDtype" + msg = "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): diff --git a/pandas/tests/indexes/period/test_constructors.py b/pandas/tests/indexes/period/test_constructors.py index 190b64ef02d23..505050a2089d8 100644 --- a/pandas/tests/indexes/period/test_constructors.py +++ b/pandas/tests/indexes/period/test_constructors.py @@ -556,19 +556,16 @@ def test_map_with_string_constructor(self): @pytest.mark.parametrize( "freq, freq_msg", [ - (offsets.BYearBegin(), ""), - (offsets.YearBegin(2), r"<2 \* YearBegins: month=1>"), - ( - offsets.QuarterBegin(startingMonth=12), - "", - ), - (offsets.BusinessMonthEnd(2), r"<2 \* BusinessMonthEnds>"), + (offsets.BYearBegin(), "BYearBegin"), + (offsets.YearBegin(2), "YearBegin"), + (offsets.QuarterBegin(startingMonth=12), "QuarterBegin"), + (offsets.BusinessMonthEnd(2), "BusinessMonthEnd"), ], ) def test_offsets_not_supported(self, freq, freq_msg): # GH#55785 msg = f"{freq_msg} is not supported as period frequency" - with pytest.raises(ValueError, match=msg): + with pytest.raises(TypeError, match=msg): Period(year=2014, freq=freq) diff --git a/pandas/tests/scalar/period/test_asfreq.py b/pandas/tests/scalar/period/test_asfreq.py index f5b17eeece000..7164de0a228d9 100644 --- a/pandas/tests/scalar/period/test_asfreq.py +++ b/pandas/tests/scalar/period/test_asfreq.py @@ -825,8 +825,8 @@ def test_asfreq_MS(self): with pytest.raises(ValueError, match=msg): initial.asfreq(freq="MS", how="S") - msg = " is not supported as period frequency" - with pytest.raises(ValueError, match=msg): + msg = "MonthBegin is not supported as period frequency" + with pytest.raises(TypeError, match=msg): Period("2013-01", "MS") assert _period_code_map.get("MS") is None diff --git a/pandas/tests/scalar/period/test_period.py b/pandas/tests/scalar/period/test_period.py index 0484a14567d28..448c2091e14f6 100644 --- a/pandas/tests/scalar/period/test_period.py +++ b/pandas/tests/scalar/period/test_period.py @@ -38,7 +38,7 @@ class TestPeriodConstruction: def test_custom_business_day_freq_raises(self): # GH#52534 - msg = "CustomBusinessDay cannot be used with Period or PeriodDtype" + msg = "CustomBusinessDay is not supported as period frequency" with pytest.raises(TypeError, match=msg): Period("2023-04-10", freq="C") with pytest.raises(TypeError, match=msg): @@ -1628,8 +1628,8 @@ def test_negone_ordinals(): def test_invalid_frequency_error_message(): - msg = " is not supported as period frequency" - with pytest.raises(ValueError, match=msg): + msg = "WeekOfMonth is not supported as period frequency" + with pytest.raises(TypeError, match=msg): Period("2012-01-02", freq="WOM-1MON") From e436a92de4d2e889e33c3ed06a495f4a788fc34b Mon Sep 17 00:00:00 2001 From: Natalia Mokeeva Date: Tue, 7 Nov 2023 14:44:06 +0100 Subject: [PATCH 6/7] add a note to v2.2.0 --- doc/source/whatsnew/v2.2.0.rst | 1 + pandas/_libs/tslibs/period.pyx | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/source/whatsnew/v2.2.0.rst b/doc/source/whatsnew/v2.2.0.rst index 2bc031e4b5f61..a99582fb5eac7 100644 --- a/doc/source/whatsnew/v2.2.0.rst +++ b/doc/source/whatsnew/v2.2.0.rst @@ -432,6 +432,7 @@ I/O Period ^^^^^^ - Bug in :class:`Period` addition silently wrapping around instead of raising ``OverflowError`` (:issue:`55503`) +- Bug in incorrectly allowing construction of :class:`Period` with certain offsets which don't have an attribute _period_dtype_code (:issue:`55785`) - Plotting diff --git a/pandas/_libs/tslibs/period.pyx b/pandas/_libs/tslibs/period.pyx index 6fcfcdf5e70e4..d1f925f3a0b48 100644 --- a/pandas/_libs/tslibs/period.pyx +++ b/pandas/_libs/tslibs/period.pyx @@ -117,7 +117,6 @@ from pandas._libs.tslibs.offsets cimport ( from pandas._libs.tslibs.offsets import ( INVALID_FREQ_ERR_MSG, BDay, - CustomBusinessDay, ) cdef: From 8fe159c736d406963151aa94c5fef3089f406a08 Mon Sep 17 00:00:00 2001 From: Natalia Mokeeva Date: Tue, 7 Nov 2023 15:06:53 +0100 Subject: [PATCH 7/7] change note in v2.2.0 --- doc/source/whatsnew/v2.2.0.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/source/whatsnew/v2.2.0.rst b/doc/source/whatsnew/v2.2.0.rst index a99582fb5eac7..38f39270d12c7 100644 --- a/doc/source/whatsnew/v2.2.0.rst +++ b/doc/source/whatsnew/v2.2.0.rst @@ -103,6 +103,7 @@ Other enhancements - :meth:`ExtensionArray.duplicated` added to allow extension type implementations of the ``duplicated`` method (:issue:`55255`) - DataFrame.apply now allows the usage of numba (via ``engine="numba"``) to JIT compile the passed function, allowing for potential speedups (:issue:`54666`) - Implement masked algorithms for :meth:`Series.value_counts` (:issue:`54984`) +- Improved error message when constructing :class:`Period` with invalid offsets such as "QS" (:issue:`55785`) .. --------------------------------------------------------------------------- .. _whatsnew_220.notable_bug_fixes: @@ -432,7 +433,6 @@ I/O Period ^^^^^^ - Bug in :class:`Period` addition silently wrapping around instead of raising ``OverflowError`` (:issue:`55503`) -- Bug in incorrectly allowing construction of :class:`Period` with certain offsets which don't have an attribute _period_dtype_code (:issue:`55785`) - Plotting