-
-
Notifications
You must be signed in to change notification settings - Fork 19.3k
#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 pandasgit diff upstream/master -u -- "*.py" | flake8 --diffBased 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 --
Timestampmakes apytz.UTCwhereas 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
datetimemore closely.I reference the python
datetimemodule documentation which defines when a datetime is naive: when itstzinfoisNoneor a call toitem.tzinfo.utcoffset(item)returnsNone.