TST: assert that informative error is raised when offset not supported as a period frequency is passed to DataFrame.asfreq #56758
Conversation
| with pytest.raises( | ||
| TypeError, match='"2MS" is not supported as a period frequency' | ||
| ): | ||
| df.asfreq(freq="2MS") |
There was a problem hiding this comment.
Can you parametrize over a BaseOffset and the freq given as a string?
There was a problem hiding this comment.
sure, although this PR isn't really about the offset cases
| ), | ||
| ( | ||
| offsets.MonthBegin(), | ||
| ValueError, |
There was a problem hiding this comment.
It looks like those are caught before they reach your new error?
There was a problem hiding this comment.
Noting that Patrick and I chatted over Slack. Summary: The change in this PR raises an error for invalid strings. Another part of the code raises errors for for invalid offsets (That was already implemented, before this PR).
aram-cinnamon
changed the title
aram-cinnamon
changed the title
|
@phofl anything I should do now to move this forward? |
There was a problem hiding this comment.
thanks for your PR - to be honest I think this validation needs doing a bit deeper, in to_offset - then a lot of other parts of the codebase could benefit from it
I think @natmokval is working on that - is it OK if we put this on pause until we've seen whether that works?
Sorry that you already spent time on this
pandas/core/arrays/period.py
Outdated
| freq = Period._maybe_convert_freq(freq) | ||
| if not hasattr(freq, "_period_dtype_code"): | ||
| raise TypeError(f'"{freq_original}" is not supported as a period frequency') |
There was a problem hiding this comment.
in #56945 (review) I'm suggesting to raise if the return value of to_offset(..., is_period=True) doesn't have a _period_dtype_code attribute
If that works, then it would resolve the linked issue too because Period._maybe_convert_freq would go down that path
|
this has been addressed by #56945 really sorry that you spent time on this one, and I hope you got something out of the experience anyway - i hate to do this, but i'll close then |
|
reopening, as:
will add a commit soon-ish (early next week) to just keep the tests, then we can merge it in I didn't get this right as reviewer, I need to own up to this |
MarcoGorelli
changed the title
…d as a period frequency is passed to DataFrame.asfreq (pandas-dev#56758) * style * add test * pytest.raises * freqstr * add test for offsets * rearrange per comment * fixup * fixup --------- Co-authored-by: MarcoGorelli <[email protected]>
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.