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.
BUG: Allow addition of Timedelta to Timestamp interval #32107
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.
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
BUG: Allow addition of Timedelta to Timestamp interval #32107
Changes from 15 commits
95f2fbe
7438110
4e2c07a
5b30740
d88aca0
48e4794
4fdc04f
ceb54d1
fe10a03
b5f169f
6cc226d
d052240
a46a0ba
784706f
63c7825
51c3c66
fa93176
9ff29a9
ab97990
4955456
e11023a
985f94e
ec57620
0c3a0ed
746f542
3d2f913
6e49272
1bc6304
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some additional
Interval
related arithmetic tests that can be moved here fromtest_interval.py
but doesn't need to be done in this PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would be somewhat clearer as
test_timestamp_interval_radd_timedelta
(or just rolled into the previous test)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we get a Timedelta interval here too? also some NaTs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to put NaT in an Interval?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intervals can't contain NA values as endpoints. It doesn't look like this gracefully handles adding
NaT
to intervals withTimestamp
/Timedelta
endpoints, but this is also true for numeric intervals, so I'm fine fixing the general case of interval arithmetic with NA in a separate PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you parametrize over delta like in the previous test? (id probably not parametrize over
__add__
and__sub__
, but your call)some more intervals that might be worth checking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the raising should also work in the other direction, i.e.
Interval(Timestamp) + numeric
should failThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated with some more cases