-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
API/DEPR: Remove +/- as setops for DatetimeIndex/PeriodIndex (GH9630) #14164
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
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 |
---|---|---|
|
@@ -731,19 +731,43 @@ def _add_datelike(self, other): | |
def _sub_datelike(self, other): | ||
# subtract a datetime from myself, yielding a TimedeltaIndex | ||
from pandas import TimedeltaIndex | ||
other = Timestamp(other) | ||
if other is tslib.NaT: | ||
result = self._nat_new(box=False) | ||
# require tz compat | ||
elif not self._has_same_tz(other): | ||
raise TypeError("Timestamp subtraction must have the same " | ||
"timezones or no timezones") | ||
if isinstance(other, DatetimeIndex): | ||
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 this condition move to 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 moved the condition to base, but left it here as well (to follow the same pattern as for the other ops: one method in base (eg |
||
# require tz compat | ||
if not self._has_same_tz(other): | ||
raise TypeError("DatetimeIndex subtraction must have the same " | ||
"timezones or no timezones") | ||
result = self._sub_datelike_dti(other) | ||
elif isinstance(other, (tslib.Timestamp, datetime)): | ||
other = Timestamp(other) | ||
if other is tslib.NaT: | ||
result = self._nat_new(box=False) | ||
# require tz compat | ||
elif not self._has_same_tz(other): | ||
raise TypeError("Timestamp subtraction must have the same " | ||
"timezones or no timezones") | ||
else: | ||
i8 = self.asi8 | ||
result = i8 - other.value | ||
result = self._maybe_mask_results(result, | ||
fill_value=tslib.iNaT) | ||
else: | ||
i8 = self.asi8 | ||
result = i8 - other.value | ||
result = self._maybe_mask_results(result, fill_value=tslib.iNaT) | ||
raise TypeError("cannot subtract DatetimeIndex and {typ}" | ||
.format(typ=type(other).__name__)) | ||
return TimedeltaIndex(result, name=self.name, copy=False) | ||
|
||
def _sub_datelike_dti(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. this is fine. but just want to check that our naming convention is for these types of routines is 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. for adding a Timedelta-like vs TimedeltaIndex, there are the method |
||
"""subtraction of two DatetimeIndexes""" | ||
if not len(self) == len(other): | ||
raise ValueError("cannot add indices of unequal length") | ||
|
||
self_i8 = self.asi8 | ||
other_i8 = other.asi8 | ||
new_values = self_i8 - other_i8 | ||
if self.hasnans or other.hasnans: | ||
mask = (self._isnan) | (other._isnan) | ||
new_values[mask] = tslib.iNaT | ||
return new_values.view('i8') | ||
|
||
def _maybe_update_attributes(self, attrs): | ||
""" Update Index attributes (e.g. freq) depending on op """ | ||
freq = attrs.get('freq', None) | ||
|
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 need to standardize here, I see @jorisvandenbossche and @sinhrks changing these!
IIRC I think we do more of
Previous Behavior
(e.g. capitalized), and american spelling (noBehaviour
). But should just pick one :)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.
yes, will do that in #14176
We indeed need to just make a choice between American or British :-)
(is capitalizing all words also an American habit? :-))
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.
I think 2 capital words sets it off s bit more
using American spelling
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.
while proofreading the whatsnew, I was thinking of putting the 'New/Old Behavior' in bold, that also sets it off a bit more
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.
even better