-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
REF: simplify Timedelta arithmetic methods #33978
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
REF: simplify Timedelta arithmetic methods #33978
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a nice clean-up! Added some questions
# Series, DataFrame, ... | ||
if other._typ == 'dateoffset' and hasattr(other, 'delta'): | ||
# Tick offset; this op will raise TypeError | ||
return other.delta * self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is covered by rmul
of DateOffset?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will go through DateOffset.__rmul__
which will raise TypeError (correctly), yes.
# i.e. np.nan, but also catch np.float64("NaN") which would | ||
# otherwise get caught by the hasattr(other, "dtype") branch | ||
# incorrectly return a np.timedelta64 object. | ||
return NaT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not checking this, does that give a change in behaviour?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the util.is_nan case is subsumed by the is_float_object case on L1351
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I see!
# i.e. np.nan, but also catch np.float64("NaN") which would | ||
# otherwise get caught by the hasattr(other, "dtype") branch | ||
# incorrectly return a np.timedelta64 object. | ||
return NaT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, subsumed by the is_float_object case on L1370
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. merge on green.
de facto green as of 3 hours ago, Travis icon hasnt updated |
In particular this avoids the "_typ" checks which go through python-space