-
-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
BUG/TST/REF: Datetimelike Arithmetic Methods #23215
Changes from 9 commits
dc2280f
37728ff
15ad0a6
94f1745
bf5e2fb
3bdf104
982ea30
d046038
a0c1a85
33d82b3
8a7c249
8e57fd8
b932121
9f3b18d
7a8232e
a743f74
0693196
29d91af
af4872e
6707032
18ef26d
60f7f8d
9372423
2a6268e
777f4d9
ee885c8
5f231b2
0214a9e
fbee9f5
f7cf3d9
d799d8e
82df39c
8cf614b
a45734a
fb007cb
1d3fd48
aecfef7
dade955
a6eb01c
acf1f74
e89e3ef
d306277
f6e4073
9146a72
4e4b9ed
f35b3b6
87e07ef
343ee30
0466b9c
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 |
---|---|---|
|
@@ -246,27 +246,6 @@ def _maybe_mask_results(self, result, fill_value=None, convert=None): | |
result[self._isnan] = fill_value | ||
return result | ||
|
||
def _nat_new(self, box=True): | ||
""" | ||
Return Array/Index or ndarray filled with NaT which has the same | ||
length as the caller. | ||
|
||
Parameters | ||
---------- | ||
box : boolean, default True | ||
- If True returns a Array/Index as the same as caller. | ||
- If False returns ndarray of np.int64. | ||
""" | ||
result = np.zeros(len(self), dtype=np.int64) | ||
result.fill(iNaT) | ||
if not box: | ||
return result | ||
|
||
attribs = self._get_attributes_dict() | ||
if not is_period_dtype(self): | ||
attribs['freq'] = None | ||
return self._simple_new(result, **attribs) | ||
|
||
# ------------------------------------------------------------------ | ||
# Frequency Properties/Methods | ||
|
||
|
@@ -347,21 +326,59 @@ def _validate_frequency(cls, index, freq, **kwargs): | |
# Arithmetic Methods | ||
|
||
def _add_datelike(self, other): | ||
# Overriden by TimedeltaArray | ||
raise TypeError("cannot add {cls} and {typ}" | ||
.format(cls=type(self).__name__, | ||
typ=type(other).__name__)) | ||
|
||
_add_datelike_dti = _add_datelike | ||
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. can you add either doc-strings or some expl here of what is going on here. Are you overridng _dti in the sub-classes? this seems like adding a lot of verbiage 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 |
||
|
||
def _sub_datelike(self, 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. after reading your other comments, I guess the _dti are simply the array-like versions? if so, then these really really need to be more explicit, e.g. something like
if not, then the others needs to be very very clear via comments & types 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 is an existing |
||
raise com.AbstractMethodError(self) | ||
# Overridden by DatetimeArray | ||
assert other is not NaT | ||
raise TypeError("cannot subtract a datelike from a {cls}" | ||
.format(cls=type(self).__name__)) | ||
|
||
_sub_datelike_dti = _sub_datelike | ||
|
||
def _sub_period(self, other): | ||
return NotImplemented | ||
# Overriden by PeriodArray | ||
raise TypeError("cannot subtract Period from a {cls}" | ||
.format(cls=type(self).__name__)) | ||
jorisvandenbossche marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
def _add_offset(self, offset): | ||
raise com.AbstractMethodError(self) | ||
|
||
def _add_delta(self, other): | ||
return NotImplemented | ||
""" | ||
Add a timedelta-like, DateOffset, or TimedeltaIndex-like object | ||
to self. | ||
|
||
Parameters | ||
---------- | ||
delta : {timedelta, np.timedelta64, DateOffset, | ||
TimedeltaIndex, ndarray[timedelta64]} | ||
|
||
Returns | ||
------- | ||
result : same type as self | ||
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. returns integers? 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. See comment immediately below the docstring. The return type is inaccurate in DatetimeLikeArrayMixin, but accurate in the subclasses. |
||
|
||
Notes | ||
----- | ||
The result's name is set outside of _add_delta by the calling | ||
method (__add__ or __sub__), if necessary (i.e. for Indexes). | ||
""" | ||
# Note: The docstring here says the return type is the same type | ||
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. this is a very confusing note here. As a reader I wouldn't know what to make of it. I would just list the possible return values here, or say same as sub-class type. 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. Will make this explicit. |
||
# as self, which is inaccurate. Once wrapped by the inheriting | ||
# Array classes, this will be accurate. | ||
|
||
if isinstance(other, (Tick, timedelta, np.timedelta64)): | ||
new_values = self._add_delta_td(other) | ||
elif is_timedelta64_dtype(other): | ||
# ndarray[timedelta64] or TimedeltaArray/index | ||
new_values = self._add_delta_tdi(other) | ||
|
||
return new_values | ||
|
||
def _add_delta_td(self, other): | ||
""" | ||
|
@@ -371,16 +388,15 @@ def _add_delta_td(self, other): | |
inc = delta_to_nanoseconds(other) | ||
new_values = checked_add_with_arr(self.asi8, inc, | ||
arr_mask=self._isnan).view('i8') | ||
if self.hasnans: | ||
new_values[self._isnan] = iNaT | ||
new_values = self._maybe_mask_results(new_values, fill_value=iNaT) | ||
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. why doesn't 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 idea. I know there is a case where we pass
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 would just change the default for now (or make it None and just fill with iNaT). i don't passing this directly (and IIRC it was never this way) 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. can you change this 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. done. |
||
return new_values.view('i8') | ||
|
||
def _add_delta_tdi(self, other): | ||
""" | ||
Add a delta of a TimedeltaIndex | ||
return the i8 result view | ||
""" | ||
if not len(self) == len(other): | ||
if len(self) != len(other): | ||
raise ValueError("cannot add indices of unequal length") | ||
|
||
if isinstance(other, np.ndarray): | ||
|
@@ -407,7 +423,9 @@ def _add_nat(self): | |
|
||
# GH#19124 pd.NaT is treated like a timedelta for both timedelta | ||
# and datetime dtypes | ||
return self._nat_new(box=True) | ||
result = np.zeros(len(self), dtype=np.int64) | ||
result.fill(iNaT) | ||
return self._shallow_copy(result, freq=None) | ||
|
||
def _sub_nat(self): | ||
"""Subtract pd.NaT from self""" | ||
|
@@ -441,7 +459,7 @@ def _sub_period_array(self, other): | |
.format(dtype=other.dtype, | ||
cls=type(self).__name__)) | ||
|
||
if not len(self) == len(other): | ||
if len(self) != len(other): | ||
raise ValueError("cannot subtract arrays/indices of " | ||
"unequal length") | ||
if self.freq != other.freq: | ||
|
@@ -635,7 +653,7 @@ def __add__(self, other): | |
result = self._addsub_offset_array(other, operator.add) | ||
elif is_datetime64_dtype(other) or is_datetime64tz_dtype(other): | ||
# DatetimeIndex, ndarray[datetime64] | ||
return self._add_datelike(other) | ||
return self._add_datelike_dti(other) | ||
elif is_integer_dtype(other): | ||
result = self._addsub_int_array(other, operator.add) | ||
elif is_float_dtype(other): | ||
|
@@ -695,7 +713,7 @@ def __sub__(self, other): | |
result = self._addsub_offset_array(other, operator.sub) | ||
elif is_datetime64_dtype(other) or is_datetime64tz_dtype(other): | ||
# DatetimeIndex, ndarray[datetime64] | ||
result = self._sub_datelike(other) | ||
result = self._sub_datelike_dti(other) | ||
elif is_period_dtype(other): | ||
# PeriodIndex | ||
result = self._sub_period_array(other) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,10 +15,10 @@ | |
from pandas._libs.tslibs.fields import isleapyear_arr | ||
|
||
from pandas import compat | ||
from pandas.util._decorators import (cache_readonly, deprecate_kwarg) | ||
from pandas.util._decorators import cache_readonly, deprecate_kwarg, Appender | ||
|
||
from pandas.core.dtypes.common import ( | ||
is_integer_dtype, is_float_dtype, is_period_dtype, is_timedelta64_dtype, | ||
is_integer_dtype, is_float_dtype, is_period_dtype, | ||
is_datetime64_dtype, _TD_DTYPE) | ||
from pandas.core.dtypes.dtypes import PeriodDtype | ||
from pandas.core.dtypes.generic import ABCSeries | ||
|
@@ -334,10 +334,6 @@ def to_timestamp(self, freq=None, how='start'): | |
|
||
_create_comparison_method = classmethod(_period_array_cmp) | ||
|
||
def _sub_datelike(self, other): | ||
assert other is not NaT | ||
return NotImplemented | ||
|
||
def _sub_period(self, other): | ||
# If the operation is well-defined, we return an object-Index | ||
# of DateOffsets. Null entries are filled with pd.NaT | ||
|
@@ -349,9 +345,7 @@ def _sub_period(self, other): | |
new_data = asi8 - other.ordinal | ||
new_data = np.array([self.freq * x for x in new_data]) | ||
|
||
if self.hasnans: | ||
new_data[self._isnan] = NaT | ||
|
||
new_data = self._maybe_mask_results(new_data, fill_value=NaT) | ||
return new_data | ||
|
||
def _add_offset(self, other): | ||
|
@@ -379,38 +373,18 @@ def _add_delta_tdi(self, other): | |
delta = self._check_timedeltalike_freq_compat(other) | ||
return self._addsub_int_array(delta, operator.add) | ||
|
||
@Appender(DatetimeLikeArrayMixin._add_delta.__doc__) | ||
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. Why this? Doesn't 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.
Will it? I thought that required some metaclass hijinx 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.
although maybe something else gets in the way? 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. The distinction is that we actually are overriding the method 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. On this branch with diff --git a/pandas/core/arrays/period.py b/pandas/core/arrays/period.py
index 1d7b57801..ff297e5bd 100644
--- a/pandas/core/arrays/period.py
+++ b/pandas/core/arrays/period.py
@@ -378,7 +378,6 @@ class PeriodArrayMixin(DatetimeLikeArrayMixin):
delta = self._check_timedeltalike_freq_compat(other)
return self._addsub_int_array(delta, operator.add)
- @Appender(DatetimeLikeArrayMixin._add_delta.__doc__)
def _add_delta(self, other):
if not isinstance(self.freq, Tick):
# We cannot add timedelta-like to non-tick PeriodArray the docstring looks fine.
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. Well, for a final check I ran it through sphinx
and it does whatever IPython is doing. The rendered docstring looks fine to. In conclusion: I think we can remove the Appender. 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. Is there some cost I'm missing to 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. Is that a common use case? I assume most people use 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 usually print the docstring, mostly because I usually forget about the other usages. This is an internal method so it doesn't really matter. My view ATM is just that its zero-cost. 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 here about a single line comment summarizing what it does |
||
def _add_delta(self, other): | ||
""" | ||
Add a timedelta-like, Tick, or TimedeltaIndex-like object | ||
to self. | ||
|
||
Parameters | ||
---------- | ||
other : {timedelta, np.timedelta64, Tick, | ||
TimedeltaIndex, ndarray[timedelta64]} | ||
|
||
Returns | ||
------- | ||
result : same type as self | ||
""" | ||
if not isinstance(self.freq, Tick): | ||
# We cannot add timedelta-like to non-tick PeriodArray | ||
raise IncompatibleFrequency("Input has different freq from " | ||
"{cls}(freq={freqstr})" | ||
.format(cls=type(self).__name__, | ||
freqstr=self.freqstr)) | ||
|
||
# TODO: standardize across datetimelike subclasses whether to return | ||
# i8 view or _shallow_copy | ||
if isinstance(other, (Tick, timedelta, np.timedelta64)): | ||
new_values = self._add_delta_td(other) | ||
return self._shallow_copy(new_values) | ||
elif is_timedelta64_dtype(other): | ||
# ndarray[timedelta64] or TimedeltaArray/index | ||
new_values = self._add_delta_tdi(other) | ||
return self._shallow_copy(new_values) | ||
else: # pragma: no cover | ||
raise TypeError(type(other).__name__) | ||
new_values = DatetimeLikeArrayMixin._add_delta(self, other) | ||
|
||
return self._shallow_copy(new_values) | ||
|
||
@deprecate_kwarg(old_arg_name='n', new_arg_name='periods') | ||
def shift(self, periods): | ||
|
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 is only used once outside of tests. Much less verbose to inline.
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.
hah I just made a comment on PeriodArray about re-using this.