Skip to content

Better error message for OOB result #32499

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Mar 11, 2020
Merged

Better error message for OOB result #32499

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
15eb9cb
Better error message for OOB result
TomAugspurger Mar 6, 2020
bb283d3
Merge remote-tracking branch 'upstream/master' into 31774-message
TomAugspurger Mar 9, 2020
815050f
wip
TomAugspurger Mar 9, 2020
6817c45
fixups
TomAugspurger Mar 9, 2020
d4aa8e8
note
TomAugspurger Mar 9, 2020
b748784
fixup
TomAugspurger Mar 10, 2020
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/v1.0.2.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ Bug fixes

- Bug in :meth:`DataFrame.reindex` and :meth:`Series.reindex` when reindexing with a tz-aware index (:issue:`26683`)
- Bug where :func:`to_datetime` would raise when passed ``pd.NA`` (:issue:`32213`)
- Improved error message when subtracting two :class:`Timestamp` that result in an invalide :class:`Timedelta` (:issue:`31774`)

**Categorical**

Expand Down
17 changes: 14 additions & 3 deletions pandas/_libs/tslibs/c_timestamp.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,9 +287,13 @@ cdef class _Timestamp(datetime):
if (PyDateTime_Check(self)
and (PyDateTime_Check(other) or is_datetime64_object(other))):
if isinstance(self, _Timestamp):
other = type(self)(other)
both_timestamps = isinstance(other, _Timestamp)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jbrockmendel can you take another look when you get a chance? Needed to change some things around to ensure that Timestamp - datetime.datetime didn't raise (since we internally convert the datetime to a Timestamp).

Copy link
Member

Choose a reason for hiding this comment

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

this looks like you're avoiding re-calling the constructor if we already have a Timestamp. Is that necessary? The constructor was recently optimized for the already-Timestamp case

Copy link
Member

Choose a reason for hiding this comment

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

never mind, i get it now

Copy link
Member

Choose a reason for hiding this comment

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

maybe add a comment to the effect of "we need to know if we started with both-Timestamps to determine whether to raise on Overflow, see GH#..."

if not both_timestamps:
other = type(self)(other)
else:
self = type(other)(self)
both_timestamps = isinstance(self, _Timestamp)
if not both_timestamps:
self = type(other)(self)

# validate tz's
if not tz_compare(self.tzinfo, other.tzinfo):
Expand All @@ -301,7 +305,14 @@ cdef class _Timestamp(datetime):
from pandas._libs.tslibs.timedeltas import Timedelta
try:
return Timedelta(self.value - other.value)
except (OverflowError, OutOfBoundsDatetime):
except (OverflowError, OutOfBoundsDatetime) as err:
if isinstance(other, _Timestamp):
if both_timestamps:
raise OutOfBoundsDatetime(
"Result is too large for pandas.Timedelta. Convert inputs "
"to datetime.datetime with 'Timestamp.to_pydatetime()' "
"before subtracting."
) from err
pass
elif is_datetime64_object(self):
# GH#28286 cython semantics for __rsub__, `other` is actually
Expand Down
14 changes: 14 additions & 0 deletions pandas/tests/scalar/timestamp/test_arithmetic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
import numpy as np
import pytest

from pandas.errors import OutOfBoundsDatetime

from pandas import Timedelta, Timestamp

from pandas.tseries import offsets
Expand Down Expand Up @@ -60,6 +62,18 @@ def test_overflow_offset_raises(self):
with pytest.raises(OverflowError, match=msg):
stamp - offset_overflow

def test_overflow_timestamp_raises(self):
# https://github.com/pandas-dev/pandas/issues/31774
msg = "Result is too large"
a = Timestamp("2101-01-01 00:00:00")
b = Timestamp("1688-01-01 00:00:00")

with pytest.raises(OutOfBoundsDatetime, match=msg):
a - b

# but we're OK for timestamp and datetime.datetime
assert (a - b.to_pydatetime()) == (a.to_pydatetime() - b)

def test_delta_preserve_nanos(self):
val = Timestamp(1337299200000000123)
result = val + timedelta(1)
Expand Down