BUG: catch overflow in timestamp + offset operations #15126
Conversation
| @@ -1220,7 +1220,10 @@ cdef class _Timestamp(datetime): | |||
| return Timestamp((self.freq * other).apply(self), freq=self.freq) | |||
|
|
|||
| elif isinstance(other, timedelta) or hasattr(other, 'delta'): | |||
| nanos = _delta_to_nanoseconds(other) | |||
| try: | |||
There was a problem hiding this comment.
cc @shoyer
since IIRC you have some expertise doing this.
I could not get this to raise the exception at all (from cython), rather it always wanted to simply call __radd__. The only way is to return NotImplemented.
any better way to do this?
There was a problem hiding this comment.
Cython really doesn't let you raise any errors from __add__ without calling __radd__ on the other object? That is surprising -- and disappointing!
It is true that Cython will use __add__ for both Python's __add__ and __radd__, so self in this method may not be a Timestamp instance:
http://cython.readthedocs.io/en/latest/src/userguide/special_methods.html#arithmetic-methods
There was a problem hiding this comment.
ok I sorted. Its very odd that it doesn't raise the actual exception.
jreback
force-pushed
the
inc
branch
2 times, most recently
from
4146495 to
04454ae
Compare
Current coverage is 85.53% (diff: 100%)@@ master #15126 diff @@
==========================================
Files 145 145
Lines 51274 51281 +7
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 43856 43865 +9
+ Misses 7418 7416 -2
Partials 0 0
|
| @@ -2748,14 +2751,20 @@ def nanos(self): | |||
|
|
|||
| def apply(self, other): | |||
| # Timestamp can handle tz and nano sec, thus no need to use apply_wraps | |||
| if isinstance(other, (datetime, np.datetime64, date)): | |||
| if isinstance(other, Timestamp): | |||
| result = other.__add__(self) | |||
There was a problem hiding this comment.
Do you really need to call __add__ here rather than using +?
Generally calling double-underscore methods directly is an anti-pattern, so if so this definitely deserves a comment to explain.
There was a problem hiding this comment.
added a comment. The + causes it to segfault. I think its because of a fast-path in the interpreter, or maybe a cython bug. If there is an exception during the the operation. Then we compound this by trying the reversed ops in .apply and the cycle repeats. But I think its too hard (maybe impossible) to change this.
xref statsmodels/statsmodels#3374 Author: Jeff Reback <[email protected]> Closes pandas-dev#15126 from jreback/inc and squashes the following commits: 923eaa2 [Jeff Reback] BUG: catch overflow in timestamp + offset operations
xref statsmodels/statsmodels#3374