Skip to content
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: Behaviour change in 1.5.0 when using Timedelta as Enum data type #49579

Merged
merged 6 commits into from Nov 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion doc/source/whatsnew/v1.5.2.rst
Expand Up @@ -19,7 +19,7 @@ Fixed regressions
from being passed using the ``colormap`` argument if Matplotlib 3.6+ is used (:issue:`49374`)
- Fixed regression in :func:`date_range` returning an invalid set of periods for ``CustomBusinessDay`` frequency and ``start`` date with timezone (:issue:`49441`)
- Fixed performance regression in groupby operations (:issue:`49676`)
-
- Fixed regression in :class:`Timedelta` constructor returning object of wrong type when subclassing ``Timedelta`` (:issue:`49579`)

.. ---------------------------------------------------------------------------
.. _whatsnew_152.bug_fixes:
Expand Down
23 changes: 12 additions & 11 deletions pandas/_libs/tslibs/timedeltas.pyx
Expand Up @@ -189,7 +189,7 @@ def ints_to_pytimedelta(ndarray m8values, box=False):
res_val = <object>NaT
else:
if box:
res_val = _timedelta_from_value_and_reso(value, reso=reso)
res_val = _timedelta_from_value_and_reso(Timedelta, value, reso=reso)
elif reso == NPY_DATETIMEUNIT.NPY_FR_ns:
res_val = timedelta(microseconds=int(value) / 1000)
elif reso == NPY_DATETIMEUNIT.NPY_FR_us:
Expand Down Expand Up @@ -741,7 +741,7 @@ cdef bint _validate_ops_compat(other):
def _op_unary_method(func, name):
def f(self):
new_value = func(self.value)
return _timedelta_from_value_and_reso(new_value, self._creso)
return _timedelta_from_value_and_reso(Timedelta, new_value, self._creso)
f.__name__ = name
return f

Expand Down Expand Up @@ -804,7 +804,7 @@ def _binary_op_method_timedeltalike(op, name):
# TODO: more generally could do an overflowcheck in op?
return NaT

return _timedelta_from_value_and_reso(res, reso=self._creso)
return _timedelta_from_value_and_reso(Timedelta, res, reso=self._creso)

f.__name__ = name
return f
Expand Down Expand Up @@ -935,10 +935,10 @@ cdef _to_py_int_float(v):


def _timedelta_unpickle(value, reso):
return _timedelta_from_value_and_reso(value, reso)
return _timedelta_from_value_and_reso(Timedelta, value, reso)


cdef _timedelta_from_value_and_reso(int64_t value, NPY_DATETIMEUNIT reso):
cdef _timedelta_from_value_and_reso(cls, int64_t value, NPY_DATETIMEUNIT reso):
# Could make this a classmethod if/when cython supports cdef classmethods
cdef:
_Timedelta td_base
Expand All @@ -949,13 +949,13 @@ cdef _timedelta_from_value_and_reso(int64_t value, NPY_DATETIMEUNIT reso):
# 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__(Timedelta, microseconds=int(value) // 1000)
td_base = _Timedelta.__new__(cls, microseconds=int(value) // 1000)
elif reso == NPY_DATETIMEUNIT.NPY_FR_us:
td_base = _Timedelta.__new__(Timedelta, microseconds=int(value))
td_base = _Timedelta.__new__(cls, microseconds=int(value))
elif reso == NPY_DATETIMEUNIT.NPY_FR_ms:
td_base = _Timedelta.__new__(Timedelta, milliseconds=0)
td_base = _Timedelta.__new__(cls, milliseconds=0)
elif reso == NPY_DATETIMEUNIT.NPY_FR_s:
td_base = _Timedelta.__new__(Timedelta, seconds=0)
td_base = _Timedelta.__new__(cls, seconds=0)
# Other resolutions are disabled but could potentially be implemented here:
# elif reso == NPY_DATETIMEUNIT.NPY_FR_m:
# td_base = _Timedelta.__new__(Timedelta, minutes=int(value))
Expand Down Expand Up @@ -1502,7 +1502,7 @@ cdef class _Timedelta(timedelta):
@classmethod
def _from_value_and_reso(cls, int64_t value, NPY_DATETIMEUNIT reso):
# exposing as classmethod for testing
return _timedelta_from_value_and_reso(value, reso)
return _timedelta_from_value_and_reso(cls, value, reso)
krasch marked this conversation as resolved.
Show resolved Hide resolved

def as_unit(self, str unit, bint round_ok=True):
"""
Expand Down Expand Up @@ -1737,7 +1737,7 @@ class Timedelta(_Timedelta):
if value == NPY_NAT:
return NaT

return _timedelta_from_value_and_reso(value, NPY_FR_ns)
return _timedelta_from_value_and_reso(cls, value, NPY_FR_ns)

def __setstate__(self, state):
if len(state) == 1:
Expand Down Expand Up @@ -1829,6 +1829,7 @@ class Timedelta(_Timedelta):
return NaT

return _timedelta_from_value_and_reso(
Timedelta,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be type(self)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same for other arithmetic methods above

Copy link
Contributor Author

@krasch krasch Nov 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had added a question re: this but somehow github ate it, weird..

Anyway, here is my concern, if I make the change you are suggesting

td1 = MyTimedeltaSubclass("10 minutes")
td2 = Timedelta ("10 minutes")

then result types will be inconsistent
td1 + td2 -> MyTimedeltaSubclass
td2 + td1 -> Timedelta

Maybe that is fine, but it also kinda weirded me out, which is why I wanted to ask you. It feels a little bit like it should be the responsibility of the subclass to implement the custom __add__ behaviour.

Wondering: does the rest of pandas respect subclasses in this way?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does the rest of pandas respect subclasses in this way?

i dont think we're very consistent about it. IIRC the stdlib timedelta has something like:

def __add__(self, other):
    if self_is_timedelta and other_is_timedelta_subclass:
        return NotImplemented
    [...]

which we could emulate. let's stick a pin in that for now and can keep Timedelta like you currently have

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I go ahead and create an issue for this?

<int64_t>(other * self.value),
reso=self._creso,
)
Expand Down
9 changes: 9 additions & 0 deletions pandas/tests/scalar/timedelta/test_constructors.py
Expand Up @@ -503,3 +503,12 @@ def test_timedelta_new_npnat():
# GH#48898
nat = np.timedelta64("NaT", "h")
assert Timedelta(nat) is NaT


def test_subclass_respected():
# GH#49579
class MyCustomTimedelta(Timedelta):
pass

td = MyCustomTimedelta("1 minute")
assert isinstance(td, MyCustomTimedelta)