-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
#31793 Add support for subtracting datetime from Timestamp #37329
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
#31793 Add support for subtracting datetime from Timestamp #37329
Conversation
If this is a welcome change, please add the |
Ok, looking at the failed tests, it l;ooks like i should be relocating the tests i added so they would appear under |
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.
Thanks @AnjoMan for the PR!
Some comments
Also heads up about linting - needs to be fixed for the CI checks to pass |
When changing this for Timestamp, we'll need to make the same change for DatetimeArray |
@jbrockmendel thanks for the heads-up, I'll take a look and try to replicate this functionality there as well. Just so we're clear, you are expecting to find the same bug when doing something like
and one of the arrays has |
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.
I -1 on adding different timezone subtraction operations. We do not allow this anywhere else. I think being explicit is a really good thing here.
5712eab
to
298431f
Compare
298431f
to
6f11284
Compare
datetime objects vs Timestamp objects isn't really applicable here. Think of DatetimeArray as being a vectorized Timestamp. Just like with Timestamp, when subtracting |
Apart from the fact that the stdlib also supports this as @jbrockmendel mentioned, I also want to point out we actually do support differing timezones in several operations, eg comparison ops:
|
6f11284
to
0e62038
Compare
This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this. |
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.
In light of this discussions in #37605 does this PR go ahead? AFAICT yes but maybe I misunderstood @jbrockmendel @jorisvandenbossche @jreback @mroeschke
If yes @AnjoMan you will need to merge master and add a whatsnew note. Not sure if this will get in in time for 1.2 - if not we'll have to wait until we tag the next major release (1.3 or 2.0)
@jorisvandenbossche and I are +1 on this, @jreback was -1 though there's some evidence of miscommunication, and @mroeschke was -1 though I'm hoping he'll reconsider when looking at this context. @arw2019 @AnjoMan I encourage you both to weigh in on #37605. |
I'm more -0 just in principle, but since we already support some mixed timezone aware operations, it's not a hill I will die on. |
I'm +1 on adding this |
@AnjoMan if you can rebase we can take another look here |
1d3fa48
to
e02cdce
Compare
Ok, i've rebased on the current dev branch and added a "whatsnew" entry. I noticed one of the CI checks fails with a bad import -- it doesn't seem too related to what I've changed |
will be fixed by #39744 |
e02cdce
to
7f48b34
Compare
27abdcc
to
c6de9d2
Compare
c6de9d2
to
26e40c7
Compare
Ok, i've updated the PR to reflect new feedback from @jbrockmendel and moved the 'whatsnew' entry to 1.4.0 |
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.
LGTM cc @jreback
26e40c7
to
c195387
Compare
@AnjoMan can you merge master one more time |
c195387
to
944dad6
Compare
@jbrockmendel i've updated the branch against pandas master |
gentle ping @jreback |
944dad6
to
498efcc
Compare
ok on board here. @AnjoMan one more rebase if you would and @mroeschke if any comments. |
6e72b91
to
ceb2426
Compare
i've rebased on top of c3d3357 |
Thanks for hanging in there @AnjoMan. Great work! |
nice! |
thanks @jbrockmendel @jreback @mroeschke for all the feedback and help getting this together! |
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff
Based on the discussion in the attached issue, i've made the following changes:
The root cause of 31793 was that the tzinfo objects were from different libraries --
Timestamp
makes apytz.UTC
whereas a datetime object might just havedatetime.timezone.utc
. These objects are not necessarily equal, even if they represent the same timezone.In addition, this change adds support for time-deltas where the timezone of each date is not the same. This follows the behaviour of
datetime
more closely.I reference the python
datetime
module documentation which defines when a datetime is naive: when itstzinfo
isNone
or a call toitem.tzinfo.utcoffset(item)
returnsNone
.