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

natmokval · 2024-01-18T20:51:27Z

xref #55785, #52064

PeriodIndex.asfreq silently converts for offsets such as offsets.MonthBegin(), offsets.BusinessMonthEnd(), etc. (with no attribute '_period_dtype_code') frequency to period frequency (in this case 'M').

Reproducible Example:

>>> import pandas as pd
>>> from pandas.tseries import offsets
>>>
>>> index = pd.PeriodIndex(["2020-01-01", "2021-01-01"], freq="Q")
>>> index.asfreq(freq=offsets.MonthBegin(1))
PeriodIndex(['2020-03', '2021-03'], dtype='period[M]')
>>>
>>> index.asfreq(freq=offsets.BusinessMonthEnd(1)) 
PeriodIndex(['2020-03', '2021-03'], dtype='period[M]')

the correct behaviour would be raising an Error:

>>> index.asfreq(freq=offsets.MonthBegin(1))
ValueError: <MonthBegin> is not supported as period frequency

Another problem: so far in the example below

>>> index = pd.PeriodIndex(["2020-01-01", "2021-01-01"], freq="M")
>>> index.asfreq(freq='BMS')

we get on main

AttributeError: 'pandas._libs.tslibs.offsets.BusinessMonthBegin' object has no attribute '_period_dtype_code'

the correct behaviour would be raising an Error:

ValueError: BMS is not supported as period frequency

added to the definition of asfreq a check if string denoting frequency is supported as period frequency. If the index of a DataFrame is a PeriodIndex and the frequency is invalid period frequency a ValueError is raised.

natmokval · 2024-01-19T09:22:43Z

@MarcoGorelli, could you please take a look at this PR?

MarcoGorelli · 2024-01-25T09:38:01Z

pandas/core/resample.py

@@ -2827,6 +2827,16 @@ def asfreq(
        if how is None:
            how = "E"

+        if isinstance(freq, str):


a few lines down, there's obj.index.asfreq(freq, how=how) - should these validation go in there?

thanks, you are right. I moved the check if isinstance(freq, str) to the definition of asfreq in pandas/core/indexes/period.py

MarcoGorelli

thanks for working on this

a lot of these validation paths go through to_offset - could we just check that if you run to_offset(..., is_period=True) then it raises an informative error message if the return value doesn't have a _period_dtype_code attribute?

So here

pandas/pandas/_libs/tslibs/offsets.pyx

Lines 4840 to 4849 in 02011f2

    
           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" 
        
               ) 
        
           elif PyDelta_Check(freq): 
        
               return delta_to_tick(freq)

instead of returning, assign the return value to a variable (say, delta, as that's what's used below)

then, just after

pandas/pandas/_libs/tslibs/offsets.pyx

Lines 4964 to 4965 in 02011f2

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

raise if is_period and delta doesn't have a _period_dtype_code attribute?

MarcoGorelli · 2024-01-31T16:20:52Z

pandas/core/indexes/period.py

+            elif offset.name == freq.replace(f"{offset.n}", ""):
+                raise ValueError(
+                    f"Invalid offset: '{offset.name}' for converting time series "
+                    f"with PeriodIndex."
+                )


I don't really understand this, but I think you don't need it - just raise ValueError(INVALID_FREQ_ERR_MSG.format(f"{freq}")) in the else branch below?

I use two different error messages here because we raise an error of invalid freq such as “ABC” and also for freq=“BMS” which is valid for offsets, but invalid for period. I agree, that it can be confusing, so better to use the standard error message f"Invalid frequency: {freq}"

MarcoGorelli · 2024-01-31T16:21:11Z

pandas/core/indexes/period.py

+                )
+            else:
+                raise ValueError(INVALID_FREQ_ERR_MSG.format(f"{freq}"))
+
        arr = self._data.asfreq(freq, how)


can you follow this asfreq and put this validation even deeper?

EDIT: as mentioned in the review, it might be better to go all the way down and do this validation within to_offset itself?

thanks, I did as you suggested and moved this validation to to_offset. It works very well.

MarcoGorelli · 2024-02-01T10:06:59Z

pandas/_libs/tslibs/offsets.pyx

-        return delta_to_tick(freq)
+        delta = delta_to_tick(freq)


maybe on line 4841, we can do

if isinstance(freq, BaseOffset): delta = freq

so then that gets validated too?

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

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")

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

thanks, I saw you already made commit for it

MarcoGorelli

nice one - does this mean it's also possible to get rid of the except in

pandas/pandas/core/resample.py

Lines 2814 to 2820 in acd914d

    
           if hasattr(freq, "_period_dtype_code"): 
        
               freq = freq_to_period_freqstr(freq.n, freq.name) 
        
           else: 
        
               raise ValueError( 
        
                   f"Invalid offset: '{freq.base}' for converting time series " 
        
                   f"with PeriodIndex." 
        
               )

?

natmokval · 2024-02-02T22:01:03Z

nice one - does this mean it's also possible to get rid of the except in

pandas/pandas/core/resample.py

Lines 2814 to 2820 in acd914d

if hasattr(freq, "_period_dtype_code"):

freq = freq_to_period_freqstr(freq.n, freq.name)

else:

raise ValueError(

f"Invalid offset: '{freq.base}' for converting time series "

f"with PeriodIndex."

)

?

I am afraid we can't rid of this block yet. When I removed this check the test_asfreq_invalid_period_freq in pandas/tests/resample/test_period_index.py failed.

Without this block we silently convert index, for example:

index = pd.PeriodIndex(["2020-01-01", "2021-01-01"], freq="Q")
df = pd.DataFrame({"a": pd.Series([0, 1], index=index)})
res = df.asfreq(freq=offsets.BusinessMonthEnd(1))
print(res.index)

PeriodIndex(['2020-03', '2021-03'], dtype='period[M]')

MarcoGorelli

should be good thanks, just one minor comment

does this change anything since 2.2? if so, I'd say that a whatsnew note in 3.0 is warranted (or in 2.2.1 if it should be backported, for example if anything introduced in 2.2 wasn't correct and needs fixing here)

MarcoGorelli · 2024-02-06T07:49:38Z

pandas/_libs/tslibs/offsets.pyx

-        return delta_to_tick(freq)
+        delta = delta_to_tick(freq)


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

MarcoGorelli · 2024-02-06T07:59:13Z

pandas/tests/scalar/period/test_period.py

-        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")


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

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")

natmokval · 2024-02-06T16:16:25Z

should be good thanks, just one minor comment

does this change anything since 2.2? if so, I'd say that a whatsnew note in 3.0 is warranted (or in 2.2.1 if it should be backported, for example if anything introduced in 2.2 wasn't correct and needs fixing here)

yes, I think we have an incorrect conversion in 2.2

>>> index = pd.PeriodIndex(["2020-01-01", "2021-01-01"], freq="M")
>>> index.asfreq(freq=offsets.MonthBegin(1))
PeriodIndex(['2020-03', '2021-03'], dtype='period[M]')
>>>
>>> index.asfreq(freq=offsets.YearBegin(1))
PeriodIndex(['2020', '2021'], dtype='period[Y-DEC]')
>>>
>>> index.asfreq(freq=offsets.BusinessMonthEnd(1))
PeriodIndex(['2020-03', '2021-03'], dtype='period[M]')

I fixed it in this PR and added an entry the 2.2.1. Now we raise a ValueError, e.g.

>>> index.asfreq(freq=offsets.MonthBegin(1))
ValueError: <MonthBegin> is not supported as period frequency

…q-invalid-period-freq

pandas/plotting/_matplotlib/timeseries.py

MarcoGorelli

wonderful, thanks a tonne @natmokval !

lumberbot-app · 2024-02-07T13:38:44Z

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

Checkout backport branch and update it.

git checkout 2.2.x
git pull

Cherry pick the first parent branch of the this PR on top of the older branch:

git cherry-pick -x -m1 cb97ce6e496e7d1df76f06550c0fe8c4590ff3ce

You will likely have some merge/cherry-pick conflict here, fix them and commit:

git commit -am 'Backport PR #56945: ENH: raise ValueError if invalid period freq pass to asfreq when the index of df is a PeriodIndex'

Push to a named branch:

git push YOURFORK 2.2.x:auto-backport-of-pr-56945-on-2.2.x

Create a PR against branch 2.2.x, I would have named this PR:

"Backport PR #56945 on branch 2.2.x (ENH: raise ValueError if invalid period freq pass to asfreq when the index of df is a PeriodIndex)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

natmokval · 2024-02-07T13:50:45Z

thanks @MarcoGorelli, for helping me with this PR!

…d freq pass to asfreq when the index of df is a PeriodIndex' (cherry picked from commit cb97ce6)

…period freq pass to asfreq when the index of df is a PeriodIndex) (#57292) 'Backport PR #56945: ENH: raise ValueError if invalid period freq pass to asfreq when the index of df is a PeriodIndex' (cherry picked from commit cb97ce6) Co-authored-by: Natalia Mokeeva <[email protected]>

…index of df is a PeriodIndex (pandas-dev#56945)

raise ValueError in asfreq for invalid period freq

39f59c4

natmokval assigned natmokval and unassigned natmokval Jan 18, 2024

natmokval added the Frequency DateOffsets label Jan 18, 2024

natmokval marked this pull request as ready for review January 18, 2024 23:02

natmokval requested a review from MarcoGorelli January 19, 2024 09:23

MarcoGorelli reviewed Jan 25, 2024

View reviewed changes

natmokval added 2 commits January 25, 2024 10:45

Merge branch 'main' into enh-error-msg-asfreq-invalid-period-freq

2b602bd

correct PeriodIndex.asfreq, fix test, add tests

ff1c6e7

MarcoGorelli requested changes Jan 31, 2024

View reviewed changes

This was referenced Jan 31, 2024

BUG: PeriodIndex.asfreq silently converts offset to period #57075

Closed

TST: assert that informative error is raised when offset not supported as a period frequency is passed to DataFrame.asfreq #56758

Merged

natmokval added 2 commits February 1, 2024 10:34

move raising ValueError to to_offset

fff7c9d

fix pre-commit errors

a389d11

MarcoGorelli reviewed Feb 1, 2024

View reviewed changes

correct def to_offset, dt64arr_to_periodarr and fix tests

d2ea914

MarcoGorelli reviewed Feb 2, 2024

View reviewed changes

fix tests

3735e23

MarcoGorelli reviewed Feb 6, 2024

View reviewed changes

natmokval added 3 commits February 6, 2024 14:43

correct def to_offset, fix tests

b1e6ba6

add a note to v2.2.1.rst

c41557a

resolve conflict

17fd84c

MarcoGorelli added 3 commits February 7, 2024 09:15

Merge remote-tracking branch 'upstream/main' into enh-error-msg-asfre…

721637b

…q-invalid-period-freq

try simplifying / clarifying

deea30d

revert _get_period_alias simplification

4cbcc5e

MarcoGorelli reviewed Feb 7, 2024

View reviewed changes

pandas/plotting/_matplotlib/timeseries.py Show resolved Hide resolved

MarcoGorelli added this to the 2.2.1 milestone Feb 7, 2024

MarcoGorelli approved these changes Feb 7, 2024

View reviewed changes

MarcoGorelli merged commit cb97ce6 into pandas-dev:main Feb 7, 2024
47 checks passed

lumberbot-app bot added the Still Needs Manual Backport label Feb 7, 2024

MarcoGorelli mentioned this pull request Feb 7, 2024

ENH: raise TypeError if offsets which are not supported as period frequency pass to asfreq #56718

Closed

3 tasks

MarcoGorelli pushed a commit to MarcoGorelli/pandas that referenced this pull request Feb 7, 2024

'Backport PR pandas-dev#56945: ENH: raise ValueError if invalid perio…

6f7aa9c

…d freq pass to asfreq when the index of df is a PeriodIndex' (cherry picked from commit cb97ce6)

natmokval mentioned this pull request Feb 7, 2024

CLN: remove from PeriodIndex.asfreq unnecessary raising ValueError for invalid period freq #57300

Merged

lithomas1 removed the Still Needs Manual Backport label Apr 10, 2024

pmhatre1 pushed a commit to pmhatre1/pandas-pmhatre1 that referenced this pull request May 7, 2024

ENH: raise ValueError if invalid period freq pass to asfreq when the …

c155b19

…index of df is a PeriodIndex (pandas-dev#56945)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

natmokval commented Jan 18, 2024 •

edited

Loading

natmokval commented Jan 19, 2024

MarcoGorelli Jan 25, 2024

natmokval Jan 25, 2024

MarcoGorelli left a comment

MarcoGorelli Jan 31, 2024

natmokval Feb 1, 2024 •

edited

Loading

MarcoGorelli Jan 31, 2024 •

edited

Loading

natmokval Feb 1, 2024

MarcoGorelli Feb 1, 2024

MarcoGorelli Feb 6, 2024

natmokval Feb 6, 2024

MarcoGorelli Feb 7, 2024

natmokval Feb 7, 2024

MarcoGorelli left a comment

natmokval commented Feb 2, 2024

MarcoGorelli left a comment

MarcoGorelli Feb 6, 2024

MarcoGorelli Feb 6, 2024

natmokval Feb 6, 2024 •

edited

Loading

natmokval commented Feb 6, 2024

MarcoGorelli left a comment

lumberbot-app bot commented Feb 7, 2024

natmokval commented Feb 7, 2024

	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"
	)

	elif PyDelta_Check(freq):
	return delta_to_tick(freq)

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

	if hasattr(freq, "_period_dtype_code"):
	freq = freq_to_period_freqstr(freq.n, freq.name)
	else:
	raise ValueError(
	f"Invalid offset: '{freq.base}' for converting time series "
	f"with PeriodIndex."
	)

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

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

Conversation

natmokval commented Jan 18, 2024 • edited Loading

natmokval commented Jan 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoGorelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natmokval Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

MarcoGorelli Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoGorelli left a comment

Choose a reason for hiding this comment

natmokval commented Feb 2, 2024

MarcoGorelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natmokval Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

natmokval commented Feb 6, 2024

MarcoGorelli left a comment

Choose a reason for hiding this comment

lumberbot-app bot commented Feb 7, 2024

natmokval commented Feb 7, 2024

natmokval commented Jan 18, 2024 •

edited

Loading

natmokval Feb 1, 2024 •

edited

Loading

MarcoGorelli Jan 31, 2024 •

edited

Loading

natmokval Feb 6, 2024 •

edited

Loading