BUG: catch overflow in timestamp + offset operations by jreback · Pull Request #15126 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation6 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
| @@ -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: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok I sorted. Its very odd that it doesn't raise the actual exception.
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
Powered by Codecov. Last update e6a3c35...923eaa2
| @@ -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) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
AnkurDedania pushed a commit to AnkurDedania/pandas that referenced this pull request
