-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
implement truediv, rtruediv directly in TimedeltaArray; tests #23829
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
Changes from all commits
6ec1f08
4c2cc59
bd2ee96
3275dd9
79901f5
adea273
da9f743
ba9e490
10bb49b
8f276ae
7d56da9
6097789
2037be8
ffedf35
2fc44aa
cd4ff57
641ad20
e0d696f
7d9e677
dfc7af4
55cad6b
d21ae78
d72bf90
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,7 @@ | |
|
||
import numpy as np | ||
|
||
from pandas._libs import algos, tslibs | ||
from pandas._libs import algos, lib, tslibs | ||
from pandas._libs.tslibs import NaT, Timedelta, Timestamp, iNaT | ||
from pandas._libs.tslibs.fields import get_timedelta_field | ||
from pandas._libs.tslibs.timedeltas import ( | ||
|
@@ -177,7 +177,7 @@ def __new__(cls, values, freq=None, dtype=_TD_DTYPE, copy=False): | |
passed=freq.freqstr)) | ||
elif freq is None: | ||
freq = inferred_freq | ||
freq_infer = False | ||
freq_infer = False | ||
|
||
result = cls._simple_new(values, freq=freq) | ||
# check that we are matching freqs | ||
|
@@ -355,12 +355,108 @@ def _evaluate_with_timedelta_like(self, other, op): | |
|
||
__mul__ = _wrap_tdi_op(operator.mul) | ||
__rmul__ = __mul__ | ||
__truediv__ = _wrap_tdi_op(operator.truediv) | ||
__floordiv__ = _wrap_tdi_op(operator.floordiv) | ||
__rfloordiv__ = _wrap_tdi_op(ops.rfloordiv) | ||
|
||
def __truediv__(self, other): | ||
# timedelta / X is well-defined for timedelta-like or numeric X | ||
other = lib.item_from_zerodim(other) | ||
|
||
if isinstance(other, (ABCSeries, ABCDataFrame, ABCIndexClass)): | ||
return NotImplemented | ||
|
||
if isinstance(other, (timedelta, np.timedelta64, Tick)): | ||
other = Timedelta(other) | ||
if other is NaT: | ||
# specifically timedelta64-NaT | ||
result = np.empty(self.shape, dtype=np.float64) | ||
result.fill(np.nan) | ||
return result | ||
|
||
# otherwise, dispatch to Timedelta implementation | ||
return self._data / other | ||
|
||
elif lib.is_scalar(other): | ||
# assume it is numeric | ||
result = self._data / other | ||
freq = None | ||
if self.freq is not None: | ||
# Tick division is not implemented, so operate on Timedelta | ||
freq = self.freq.delta / other | ||
return type(self)(result, freq=freq) | ||
|
||
if not hasattr(other, "dtype"): | ||
# e.g. list, tuple | ||
other = np.array(other) | ||
|
||
if len(other) != len(self): | ||
raise ValueError("Cannot divide vectors with unequal lengths") | ||
|
||
elif is_timedelta64_dtype(other): | ||
# let numpy handle it | ||
return self._data / other | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to ensure There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but that is another level of redirection, while we know this will happen and can directly do the correct thing here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, since we exclude Series and Index at the start.
I guess we could replace There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the (hopefully not too distant future), TimedeltaArray will no longer be an Index. In this case we would want to explicitly grab the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And what's the return type here? Does this need to be wrapped in a a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
float-dtyped ndarray |
||
|
||
elif is_object_dtype(other): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to allow this? (I first wanted to say: can't we dispatch that to numpy, thinking that numpy object dtype would handle that, but they raise a TypeError) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can see why this would be first on the chopping block if we had to support fewer cases. Is there a compelling reason not to handle this case? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can also turn around the question :) Is there a compelling reason to do handle this case? It's just an extra case to support. And eg, we could discuss whether this should return object dtype data or timedelta, as you are inferring now? Looking at Series behaviour with int64 and object integers, it actually returns object. For datetimes it now raises. So at least, our support is at the moment not very consistent. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Because a selling point of pandas is that things Just Work? Because the code and tests are already written, so the marginal cost is \approx zero?
Fair enough. If a goal is to make things more consistent (which I'm +1 on BTW) then we're probably not going to go around and start breaking the places where it currently is supported. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i agree with @jbrockmendel here |
||
# Note: we do not do type inference on the result, so either | ||
# an object array or numeric-dtyped (if numpy does inference) | ||
# will be returned. GH#23829 | ||
result = [self[n] / other[n] for n in range(len(self))] | ||
result = np.array(result) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actually this is really close to what soft_convert_objects does. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That isn't clear to me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you are essentially re-implementing it. i would rather not do that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not clear on what you have in mind. Something like:
? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough, will change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. changed. this ends up changing the behavior of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is changed here? shouldn't is_object_type result in a TypeError or a NotImplemented? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are two independent questions that have been asked about the object-dtype case:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok this is fine, you are returning object dtype (which is consistent with how we do for Series now) |
||
return result | ||
|
||
else: | ||
result = self._data / other | ||
return type(self)(result) | ||
|
||
def __rtruediv__(self, other): | ||
# X / timedelta is defined only for timedelta-like X | ||
other = lib.item_from_zerodim(other) | ||
|
||
jbrockmendel marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if isinstance(other, (ABCSeries, ABCDataFrame, ABCIndexClass)): | ||
return NotImplemented | ||
|
||
if isinstance(other, (timedelta, np.timedelta64, Tick)): | ||
other = Timedelta(other) | ||
if other is NaT: | ||
# specifically timedelta64-NaT | ||
result = np.empty(self.shape, dtype=np.float64) | ||
result.fill(np.nan) | ||
return result | ||
|
||
# otherwise, dispatch to Timedelta implementation | ||
return other / self._data | ||
|
||
elif lib.is_scalar(other): | ||
raise TypeError("Cannot divide {typ} by {cls}" | ||
.format(typ=type(other).__name__, | ||
cls=type(self).__name__)) | ||
|
||
if not hasattr(other, "dtype"): | ||
# e.g. list, tuple | ||
other = np.array(other) | ||
|
||
if len(other) != len(self): | ||
raise ValueError("Cannot divide vectors with unequal lengths") | ||
|
||
elif is_timedelta64_dtype(other): | ||
# let numpy handle it | ||
return other / self._data | ||
|
||
elif is_object_dtype(other): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or can raise NotImplemented here? does that work? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that might be fragile; might depend on having There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same comment as above There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, can you add a comment here (and above), were we do the operation but do not infer the output type (just for posterity), otherwise this PR lgtm. ping on green. |
||
# Note: unlike in __truediv__, we do not _need_ to do type# | ||
# inference on the result. It does not raise, a numeric array | ||
# is returned. GH#23829 | ||
result = [other[n] / self[n] for n in range(len(self))] | ||
return np.array(result) | ||
|
||
else: | ||
raise TypeError("Cannot divide {dtype} data by {cls}" | ||
.format(dtype=other.dtype, | ||
cls=type(self).__name__)) | ||
|
||
if compat.PY2: | ||
__div__ = __truediv__ | ||
__rdiv__ = __rtruediv__ | ||
|
||
# Note: TimedeltaIndex overrides this in call to cls._add_numeric_methods | ||
def __neg__(self): | ||
|
Uh oh!
There was an error while loading. Please reload this page.