BUG: #29049 make holiday support offsets of offsets #57938
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dontgoto
changed the title
mroeschke
requested changes
Mar 25, 2024
pandas/tseries/holiday.py
Outdated
| @@ -159,24 +166,34 @@ def __init__( | |||
| year=None, | |||
| month=None, | |||
| day=None, | |||
| offset=None, | |||
| observance=None, | |||
| offset: BaseOffset | list[BaseOffset | list[BaseOffset]] | None = None, | |||
There was a problem hiding this comment.
I don't think list[list[BaseOffset]] should be supported. It's not worth the complexity when the provided list[BaseOffset] can be spat into a new argument
There was a problem hiding this comment.
I removed the logic for handling this case and instead only throw a more informative error now. The type hint alone might already be sufficient for when someone chains Holiday.offset without considering that they can be lists, but I think the more detailed exception could also be nice to have.
…ets_of_offsets' into #29049_make_holiday_support_offsets_of_offsets
mroeschke
reviewed
Mar 27, 2024
mroeschke
reviewed
Mar 27, 2024
pandas/tseries/holiday.py
Outdated
| @@ -149,6 +156,7 @@ class Holiday: | |||
| for observance. | |||
| """ | |||
|
|
|||
| offset: BaseOffset | list[BaseOffset] | None | |||
There was a problem hiding this comment.
Shouldn't mypy be able to infer this?
There was a problem hiding this comment.
It does. I removed this part.
mroeschke
reviewed
Mar 27, 2024
pandas/tseries/holiday.py
Outdated
Comment on lines
237
to
239
| if offset is not None and observance is not None: | ||
| raise NotImplementedError("Cannot use both offset and observance.") | ||
| if isinstance(offset, list) and any(isinstance(off, list) for off in offset): |
There was a problem hiding this comment.
Suggested change
| if offset is not None and observance is not None: | |
| raise NotImplementedError("Cannot use both offset and observance.") | |
| if isinstance(offset, list) and any(isinstance(off, list) for off in offset): | |
| if offset is not None: | |
| if observance is not None: | |
| raise NotImplementedError("Cannot use both offset and observance.") | |
| if not (isinstance(offset, DateOffset) or (isinstance(list, DateOffset) and all(isinstance(off, DateOffset) for off in offsets))): |
mroeschke
reviewed
Mar 27, 2024
pandas/tseries/holiday.py
Outdated
Comment on lines
241
to
242
| "Nested lists are not supported for offset. " | ||
| "Flatten composite offsets of `Holiday.offset`s first." |
There was a problem hiding this comment.
This should probably just say what is allowed for offset instead of a specific failure case
Co-authored-by: Matthew Roeschke <[email protected]>
…ets_of_offsets' into #29049_make_holiday_support_offsets_of_offsets
mroeschke
approved these changes
Mar 28, 2024
|
Thanks @dontgoto |
|
Thank you for the input @mroeschke |
pmhatre1
pushed a commit
to pmhatre1/pandas-pmhatre1
that referenced
this pull request
May 7, 2024
…-dev#57938) * support offsets of offsets in holiday creation * update whatsnew * add type annotations * update type annotation * make types compatible * update docstring * don't accept, only raise with more informative exception * add attribute type hint * change array to list in docstring Co-authored-by: Matthew Roeschke <[email protected]> * broaden if check; specify error message * update test raises message --------- Co-authored-by: Matthew Roeschke <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
doc/source/whatsnew/v3.0.0.rstfile if fixing a bug or adding a new feature.The summing of offsets is generally not possible, so the bug is fixed by simply flattening the asymmetric list of lists of offsets that one might easily end up with when defining holidays in relation to each other.
In theory it is possible to sum consecutive
DataOffset(n, weekday=None)in the offset list, but I did not want to introduce even more complexity for such little gain.Allowing asymmetrical lists of lists and their element dtypes as parameters is not very pretty, but the only solution to this problem that does not involve a major refactoring of the offset classes to allow representation of lists of offsets in a single composite offset e.g. ["TUE(1)", 3]. Summing of composite offsets is not possible because additions between weekday and regular day shift offsets are not associative, not distributive, and not commutative.
Another option would be to just enhance the error message given to the end user when trying to chain offsets in the manner shown in the issue and let them do the aggregation to a simple list of offsets themselves.