Skip to content

BUG: Tick division is incorrect #57508

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

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/source/whatsnew/v2.2.1.rst
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ Fixed regressions
- Fixed regression in :meth:`Index.join` raising ``TypeError`` when joining an empty index to a non-empty index containing mixed dtype values (:issue:`57048`)
- Fixed regression in :meth:`Series.pct_change` raising a ``ValueError`` for an empty :class:`Series` (:issue:`57056`)
- Fixed regression in :meth:`Series.to_numpy` when dtype is given as float and the data contains NaNs (:issue:`57121`)
- Fixed regression where dividing a :class:`tseries.offsets.Tick` by a value greater than its :attr:`tseries.offsets.Tick.n` would incorrectly return 0 instead of changing to a higher resolution (:issue:`57264`)

.. ---------------------------------------------------------------------------
.. _whatsnew_221.bug_fixes:
Expand Down
20 changes: 19 additions & 1 deletion pandas/_libs/tslibs/timedeltas.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ from pandas._libs.tslibs.np_datetime cimport (
cmp_dtstructs,
cmp_scalar,
convert_reso,
get_conversion_factor,
get_datetime64_unit,
get_unit_from_dtype,
import_pandas_datetime,
Expand Down Expand Up @@ -965,6 +966,7 @@ cdef _timedelta_from_value_and_reso(cls, int64_t value, NPY_DATETIMEUNIT reso):
# 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:
Expand Down Expand Up @@ -2047,6 +2049,9 @@ class Timedelta(_Timedelta):
__rmul__ = __mul__

def __truediv__(self, other):
cdef:
NPY_DATETIMEUNIT curr_unit, next_unit
int64_t curr_value
if _should_cast_to_timedelta(other):
# We interpret NaT as timedelta64("NaT")
other = Timedelta(other)
Expand All @@ -2066,8 +2071,21 @@ class Timedelta(_Timedelta):
other = int(other)
if isinstance(other, cnp.floating):
other = float(other)

# If we have a non-nano timedelta and
# self._value < other, we would get 0 as a result,
# which is not correct.
# We need to try going to a higher resolution to fix this
curr_value = self._value
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not the happiest with this code, since there's a possibility we might overflow (if other is very big), and there could be the possibility that a Tick is not representable in nanoseconds.

Is this a real possibility?
(not too familiar with tick code, so I can't tell mysel)

Copy link
Member

Choose a reason for hiding this comment

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

this introduces (untested) changes in Timedelta division in addition to Tick. I'm skeptical that this is the right place for a change, but if it is, it should be tested.

curr_unit = self._creso
while curr_value < other and curr_unit < NPY_FR_ns:
next_unit = <NPY_DATETIMEUNIT>(<int64_t>curr_unit + 1)
cf = get_conversion_factor(curr_unit, next_unit)
curr_value *= cf
curr_unit = next_unit

return Timedelta._from_value_and_reso(
<int64_t>(self._value/ other), self._creso
<int64_t>(curr_value / other), curr_unit
)

elif is_array(other):
Expand Down
5 changes: 5 additions & 0 deletions pandas/tests/tseries/offsets/test_ticks.py
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,11 @@ def test_tick_division(cls):
result = off / 1000
assert isinstance(result, offsets.Tick)
assert not isinstance(result, cls)
# GH57264
# Test that roundtripping works
# to ensure that the timedelta division isn't giving something wrong
# (e.g. 0)
assert result * 1000 == off
assert result._as_pd_timedelta == off._as_pd_timedelta / 1000

if cls._nanos_inc < Timedelta(seconds=1)._value:
Expand Down