-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
TST: Extend timedelta64 arithmetic tests to TimedeltaArray #23642
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 10 commits
e6463be
f5a7891
291dd51
52319fd
da1b45a
0b21797
cb63fe7
6ccff81
1ffc21f
8c9f296
3c53183
74046ba
1c9fb1a
9f4d4d9
07c858e
b8e2127
6dd6b3b
9c11a6c
e4c7e98
cf97665
62edbc3
fa3e8d1
4cc3cbd
2df7d65
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 |
---|---|---|
@@ -1,5 +1,8 @@ | ||
# -*- coding: utf-8 -*- | ||
from __future__ import division | ||
|
||
from datetime import timedelta | ||
import operator | ||
import warnings | ||
|
||
import numpy as np | ||
|
@@ -22,11 +25,13 @@ | |
is_datetime64_dtype, | ||
is_list_like, | ||
ensure_int64) | ||
from pandas.core.dtypes.generic import ABCSeries, ABCTimedeltaIndex | ||
from pandas.core.dtypes.generic import ( | ||
ABCDataFrame, ABCSeries, ABCTimedeltaIndex, ABCIndexClass) | ||
from pandas.core.dtypes.missing import isna | ||
|
||
import pandas.core.common as com | ||
from pandas.core.algorithms import checked_add_with_arr | ||
from pandas.core import ops | ||
|
||
from pandas.tseries.offsets import Tick | ||
from pandas.tseries.frequencies import to_offset | ||
|
@@ -107,8 +112,32 @@ def wrapper(self, other): | |
return compat.set_function_name(wrapper, opname, cls) | ||
|
||
|
||
def _wrap_tdi_op(op): | ||
""" | ||
Instead of re-implementing multiplication/division etc operations | ||
in the Array class, for now we dispatch to the TimedeltaIndex | ||
implementations. | ||
""" | ||
# TODO: implement directly here and wrap in TimedeltaIndex, instead of | ||
# the other way around | ||
def method(self, other): | ||
if isinstance(other, (ABCSeries, ABCDataFrame, ABCIndexClass)): | ||
return NotImplemented | ||
|
||
from pandas import TimedeltaIndex | ||
obj = TimedeltaIndex(self) | ||
result = op(obj, other) | ||
if is_timedelta64_dtype(result): | ||
return type(self)(result) | ||
return np.array(result) | ||
|
||
method.__name__ = '__{name}__'.format(name=op.__name__) | ||
return method | ||
|
||
|
||
class TimedeltaArrayMixin(dtl.DatetimeLikeArrayMixin): | ||
_typ = "timedeltaarray" | ||
__array_priority__ = 1000 | ||
|
||
@property | ||
def _box_func(self): | ||
|
@@ -288,6 +317,21 @@ def _evaluate_with_timedelta_like(self, other, op): | |
|
||
return NotImplemented | ||
|
||
__mul__ = _wrap_tdi_op(operator.mul) | ||
__rmul__ = __mul__ | ||
TomAugspurger marked this conversation as resolved.
Show resolved
Hide resolved
|
||
__truediv__ = _wrap_tdi_op(operator.truediv) | ||
__floordiv__ = _wrap_tdi_op(operator.floordiv) | ||
__rfloordiv__ = _wrap_tdi_op(ops.rfloordiv) | ||
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. how are you implmenting rfloordiv but not rdiv / rtruediv? 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. very specifically implementing only the methods needed to extend the affected tests. I'll double-check why i couldn't extend tests for rdiv and rtruediv 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 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. @jbrockmendel any findings 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. AFAICT we don't have test cases for the other methods, at least not in tests.arithmetic.test_timedelta64. I've scoured the other test files to centralize arithmetic tests, but it's conceivable I've missed some. We definitely need such tests, but I'd rather do that in a dedicated PR, and would prefer not to implement the methods here without the corresponding tests. 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.
+1 |
||
|
||
if compat.PY2: | ||
__div__ = __truediv__ | ||
jorisvandenbossche marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
# Note: TimedeltaIndex overrides this in call to cls._add_numeric_methods | ||
def __neg__(self): | ||
if self.freq is not None: | ||
return type(self)(-self._data, freq=-self.freq) | ||
return type(self)(-self._data) | ||
|
||
jbrockmendel marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# ---------------------------------------------------------------- | ||
# Conversion Methods - Vectorized analogues of Timedelta methods | ||
|
||
|
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.
we should use the already existing machinery and simply implement this
_create_arithmetic_method
and a limited form of:
otherwise you have different styles here than elsewhere.
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.
We use this pattern in plenty of places: nattype, Timedelta, field accessors TDA/DTA/PA, ... Using the classmethod in this context doesn't add anything except a layer of indirection.
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.
right but that'st the point, we should be using this here as this IS using the mixin that implements this. This is creating yet another pattern.
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.
No, it isn't. It inherits from ExtensionOpsMixin to use _add_comparison_comps, NOT _add_arithmetic_ops. In fact if were to try to use _add_arithmetic_ops that would be working at cross-purposes with
_add_datetimelike_methods
, which specifically implements add/sub-like methods.I'm as big a fan of internal consistency and de-duplication as the next guy, but this isn't the place to make that stand.
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.
@jreback ok for now?
(anyway, I think the goal is to rework this a bit at some point, to not wrap TimedeltaIndex here, rather the other way around)
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.
Correct