BUG: Timedelta created by to_timedelta does not add correctly to datetime in Pandas 2

[Flix6x]

Pandas version checks

• I have checked that this issue has not already been reported.

• I have confirmed this bug exists on the latest version of pandas.

• I have confirmed this bug exists on the main branch of pandas.

Reproducible Example

from datetime import datetime
import pandas as pd

# Create a datetime object (midnight)
dt = datetime(2015, 1, 1, 0)

# Create a pd.Timedelta object (1 hour)
delta_1 = pd.Timedelta("1H")

# Create a pd.Timedelta object from a DateTimeIndex frequency (1 hour)
delta_2 = pd.to_timedelta(pd.date_range('2015-01-01 00', '2015-01-01 01', freq="1H").freq)

# First check that the pd.Timedelta objects are equal
assert(delta_1 == delta_2)  # True

# Then check whether adding each pd.Timedelta to the datetime object moves the datetime by one hour: this does not behave correctly since Pandas 2.0
assert(dt + delta_1 == dt + delta_2)  # False -> AssertionError

# Left-hand side looks right: the hour was added
dt + delta_1  # datetime.datetime(2015, 1, 1, 1, 0)

# Right-hand side appears to be wrong: the hour was not added
dt + delta_2  # datetime.datetime(2015, 1, 1, 0, 0)

Issue Description

It seems that the pd.Timedelta object produced by calling pd.to_timedelta on a pd.DateTimeIndex frequency can no longer be added to a datetime.datetime object since pandas>1.5.3.

It's not evident to me why the two pd.Timedelta objects (appearing to be equal) are behaving differently.

Expected Behavior

Running the above example in pandas==1.5.3 (and also 1.4.4) works as expected.

Installed Versions

INSTALLED VERSIONS

commit : 4149f32
python : 3.9.9.final.0
python-bits : 64
OS : Linux
OS-release : 5.15.11-76051511-generic
Version : #202112220937164018548121.04~b3a2c21-Ubuntu SMP Mon Jan 3 16:5
machine : x86_64
processor :
byteorder : little
LC_ALL : None
LANG : en_US.UTF-8
LOCALE : en_US.UTF-8
pandas : 2.1.0.dev0+956.g4149f32388
numpy : 1.22.4
pytz : 2022.2.1
dateutil : 2.8.2
setuptools : 65.2.0
pip : 23.1.2
Cython : 0.29.30
pytest : 7.0.1
hypothesis : None
sphinx : 4.4.0
blosc : None
feather : None
xlsxwriter : None
lxml.etree : None
html5lib : 1.1
pymysql : None
psycopg2 : 2.9.3
jinja2 : 3.1.2
IPython : None
pandas_datareader: None
bs4 : 4.11.1
bottleneck : None
brotli : None
fastparquet : None
fsspec : None
gcsfs : None
matplotlib : 3.5.1
numba : 0.56.0
numexpr : None
odfpy : None
openpyxl : 3.0.10
pandas_gbq : None
pyarrow : None
pyreadstat : None
pyxlsb : None
s3fs : None
scipy : 1.8.0
snappy : None
sqlalchemy : 1.4.31
tables : None
tabulate : 0.8.9
xarray : None
xlrd : 2.0.1
zstandard : None
tzdata : 2022.5
qtpy : None
pyqt5 : None

[jbrockmendel]

I have an explanation but not a solution.

delta_2 here has unit="s". Timedeltas with unit="s" can go outside the range supported by datetime.timedelta, so the way they are initialized passes seconds=0 to the parent class (datetime.timedelta) constructor. So the pytimedelta struct (which pandas ignores) looks like timedelta(0). When you do dt + delta_2, that calls the pydatetime's __add__ method, which treats delta_2 as a regular pytimedelta, not a pd.Timedelta. So it uses the 0, giving the incorrect result.

Reversing the operation and doing delta_2 + dt gives the correct result, as does pd.Timestamp(dt) + delta_2.

A solution here would need to be in the stdlib datetime.__add__ returning NotImplemented

[miccoli]

A solution here would need to be in the stdlib datetime.__add__ returning NotImplemented

No way! pd.Timedelta claims to be a datetime.timedelta subclass:

>>> import datetime
>>> import pandas as pd
>>> issubclass(pd.Timedelta, datetime.timedelta)
True

The Liskov substitution principle requires that datetime.__add__(self, other) is semantically equivalent, whenever other is a datetime.timedelta subclass.

By the way, why is pd.Timedelta a datetime.timedelta subclass? They are mostly incompatible. The only way forward seems to me to completely drop the claim that pd.Timedelta should be a datetime.timedelta subclass.

[GiorgioBalestrieri]

Just caught a bug caused by this while trying to upgrade to pandas 2.0. This is both quite concerning (as mentioned above, pd.Timedelta should behave like dt.timedelta, since it claims to be a subclass), and potentially a blocker, because we'll have to go and ensure that each single pd.Timedelta is converted to dt.timedelta before doing any arithmetics on it.

I really don't want to sound like I'm demanding a fix for a bug/issue in an open source library/project, but I think solving this should be priority.

[Flix6x]

Maybe a silly suggestion, but could we not simply have pd.Timedelta.__add__ conditionally switch the order of self and other?

A solution here would need to be in the stdlib datetime.__add__ returning NotImplemented

Could you reformulate this solution as an issue with the stdlib? As it stands, I don't understand what would be wrong there.

They are mostly incompatible.

Since pd.Timedelta is subclassing datetime.timedelta I would actually expect they are mostly compatible. Could you elaborate on this?

[miccoli]

There is no easy solution, right now: the problem is with datetime.datetime.__add__ (and not not with pd.Timedelta.__add__).

See this example:

import datetime
import pandas as pd

dt = datetime.datetime(2015, 1, 1, 0)
delta_2 = pd.to_timedelta(pd.date_range('2015-01-01 00', '2015-01-01 01', freq="1h").freq)
assert isinstance(delta_2, datetime.timedelta)

print(dt.__add__(delta_2))  # 2015-01-01 00:00:00
print(delta_2.__add__(dt))  # 2015-01-01 01:00:00

So python stdlib datetime.datetime.__add__ method will see that delta_2 is a datetime.timedelta instance and assume that it behaves like datetime.timedelta(seconds=3600), which is not true.

As a general comment, maybe datetime.datetime.__add__ is a little too optimistic, since for efficiency it assumes that the pytimedelta struct holds a valid value, while of course it could also query the higher level property pd.Timedelta.seconds...

By the way, there is a generale feature of Python that allows subclasses to override their ancestors’ operations, see the note at the end of https://docs.python.org/3/reference/datamodel.html#emulating-numeric-types.

In fact

delta_1 = datetime.timedelta(seconds=3600)
print(delta_1 + delta_2)  # 0 days 02:00:00

works as expected, because delta_1 + delta_2 will call the reflected delta_2.__radd__(delta_1) first, even if delta_1.__add__(delta_2) does not return NotImplemented

repr(delta_1 + delta_2)  # "Timedelta('0 days 02:00:00')"
repr(delta_1.__add__(delta_2))  # 'datetime.timedelta(seconds=3600)'
repr(delta_2.__radd__(delta_1))  # "Timedelta('0 days 02:00:00')"

So maybe a way forward is to convince the CPython developers that special code should be added to datetime.datetime.__add__(x) to check first for a reflected operator if type(x) is a subclass of datetime.timedelta... but I think that it is highly improbable that a fix will be accepted form the side of CPython.

From the side of pandas, the obvious solution would be to ensure that issubclass(pandas._libs.tslibs.timedeltas.Timedelta, datetime.timedelta) returns false, so that CPython is forced to use the reflected

delta_2.__radd__(dt)  # Timestamp('2015-01-01 01:00:00')

which works as expected.

Bottom line: do not mix python datetime.* objects with pandas time objects, because they are largely incompatible, and could give raise to strange behaviour.

[giuliobeseghi]

Do we understand why this wasn't an issue in pandas<2.0?

[GiorgioBalestrieri]

Timedeltas with unit="s" can go outside the range supported by datetime.timedelta

curious: what is the use case for this? If datetime arithmetic is not guaranteed to work anyway, wouldn't it be reasonable to constraint pd.Timedelta to the same range of values of datetime.timedelta, and represent the timedelta object "correctly" internally, which would effectively solve this issue?

Code in question:

pandas/pandas/_libs/tslibs/timedeltas.pyx

Lines 965 to 976 in adec21f

	# For millisecond and second resos, we cannot actually pass int(value) because
	# many cases would fall outside of the pytimedelta implementation bounds.
	# We pass 0 instead, and override seconds, microseconds, days.
	# In principle we could pass 0 for ns and us too.
	if reso == NPY_FR_ns:
	td_base = _Timedelta.__new__(cls, microseconds=int(value) // 1000)
	elif reso == NPY_DATETIMEUNIT.NPY_FR_us:
	td_base = _Timedelta.__new__(cls, microseconds=int(value))
	elif reso == NPY_DATETIMEUNIT.NPY_FR_ms:
	td_base = _Timedelta.__new__(cls, milliseconds=0)
	elif reso == NPY_DATETIMEUNIT.NPY_FR_s:
	td_base = _Timedelta.__new__(cls, seconds=0)

This was introduced in: #47641