#31793 Add support for subtracting datetime from Timestamp
State: closed

- [x] closes #31793
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

Based on the discussion in the attached issue, i've made the following changes:

1. add support for subtracting when the two items have different timezones
2. raise an error if one (but not both) timezone is naive

The root cause of 31793 was that the tzinfo objects were from different libraries -- `Timestamp` makes a `pytz.UTC` whereas a datetime object might just have `datetime.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](https://docs.python.org/3/library/datetime.html) which defines when a datetime is naive: when its `tzinfo` is `None` or a call to `item.tzinfo.utcoffset(item)` returns `None`.

Labels:
name: Bug
name: Dtype Conversions
description: Unexpected or buggy dtype conversions
name: Timezones
description: Timezone data dtype

Comments:
If this is a welcome change, please add the `hacktoberfest-accepted` to this issue so it can go towards my event contribution
Ok, looking at the failed tests, it l;ooks like i should be relocating the tests i added so they would appear under `tests/arithmetic/test_timedetlta64.py` and `tests/scalar/timestamp/test_arithmetic.py`.
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
>  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
```
date_array_1 = DatetimeArray(...)
date_array_2 = DatetimeArray(...)

date_array_1 - date_array_2
```
and one of the arrays has `datetime` objects instead of `Timestamp` objects?
Hello @AnjoMan! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:









There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers: 

##### Comment last updated at 2021-12-28 15:51:50 UTC
> Just so we're clear, you are expecting to find the same bug when doing something like
> ```
> date_array_1 = DatetimeArray(...)
> date_array_2 = DatetimeArray(...)
> date_array_1 - date_array_2
> ```
> and one of the arrays has datetime objects instead of Timestamp objects?

datetime objects vs Timestamp objects isn't really applicable here.  Think of DatetimeArray as being a vectorized Timestamp.  Just like with Timestamp, when subtracting `dta - dta2` we need to have `not ((dta.tzinfo is None) ^ (dta2.tzinfo is None))`.  ATM we also require that the tzinfos match, which this PR would cease to require.
> [@jreback] I am -1 on adding different timezone subtraction operations. We do not allow this anywhere else. I think being explicit is a really good thing here.

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:

```
In [5]: t1 = pd.Timestamp("2012-01-01").tz_localize("UTC").tz_convert("Europe/Brussels")

In [6]: t2 = pd.Timestamp("2012-01-01").tz_localize("UTC").tz_convert("US/Eastern")

In [7]: t1 == t2
Out[7]: True

In [8]: t1 >= t2
Out[8]: True
```
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.
> In light of this discussions in #37605 does this PR go ahead? AFAICT yes but maybe I misunderstood

@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.
> > In light of this discussions in #37605 does this PR go ahead? AFAICT yes but maybe I misunderstood
> 
> @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 +1 on adding this
@AnjoMan if you can rebase we can take another look here
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](https://github.com/pandas-dev/pandas/pull/37329/checks?check_run_id=1876403833) -- it doesn't seem too related to what I've changed

![image](https://user-images.githubusercontent.com/2414360/107601671-645e6700-6bf5-11eb-8b5c-a11a2e9a925c.png)

> 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](https://github.com/pandas-dev/pandas/pull/37329/checks?check_run_id=1876403833) -- it doesn't seem too related to what I've changed
> 
> ![image](https://user-images.githubusercontent.com/2414360/107601671-645e6700-6bf5-11eb-8b5c-a11a2e9a925c.png)

will be fixed by #39744
Hi is there anything further that needs to be done here?
> Hi is there anything further that needs to be done here?

Nothing else required on this AFAIK, other than @jreback has a review request that i think has been adressed
It looks like there are still a few failing tests i need to adress -- from what i can tell the main issue is that some tests expect a slightly different error message.
Ok, i think this is back on track -- there is one last failing check but it is a mypy issue that I'm fairly certain is not related to my changes at all:

![image](https://user-images.githubusercontent.com/2414360/110995112-cd95df00-8347-11eb-9fc1-2bec4fce266f.png)

it seems likely that the mypy complaint is unrelated.  can you merge master and ill take another look
@AnjoMan can you address @jbrockmendel comments
I've updated the PR to address feedback from @jbrockmendel 

1. In `pandas/_libs/tslibs/timestamps.pyx` we raise 'cannot _subtract_' instead of 'cannot _compare_' as it is more specific (in other cases we still raise 'cannot _compare_' as array / series implementations of __sub__ have an awareness check that is optimized for vectors of timestamps
2. add a missing assertion in `pandas/tests/series/methods/test_shift.py `
3. add coverage in `pandas/tests/arithmetic/test_datetime64.py` of the failure case when subtracting between series/array and series/array/timestamp
4. reworded whatsnew.rst entry
in theory ok with this, if you can merge master and will look
@jreback i updated it
Ok, i've updated the PR to reflect new feedback from @jbrockmendel and moved the 'whatsnew' entry to 1.4.0
@AnjoMan can you merge master one more time
@jbrockmendel i've updated the branch against pandas master
gentle ping @jreback 
ok on board here. @AnjoMan one more rebase if you would and @mroeschke if any comments.
i've rebased on top of c3d335735d9f201910ddef709af3fbc55252bec2