BUG: Fix PeriodIndex +/- TimedeltaIndex #23031
Conversation
|
Hello @jbrockmendel! Thanks for updating the PR.
Comment last updated on October 09, 2018 at 23:56 Hours UTC |
| @@ -805,6 +805,7 @@ def assert_index_equal(left, right, exact='equiv', check_names=True, | |||
| Specify object name being compared, internally used to show appropriate | |||
| assertion message | |||
| """ | |||
| __tracebackhide__ = True | |||
pandas/core/arrays/datetimelike.py
Outdated
| # Explicitly catch invalid dtypes | ||
| raise TypeError("cannot add {dtype}-dtype to {cls}" | ||
| .format(dtype=other.dtype, | ||
| cls=type(self).__name__)) | ||
| elif is_period_dtype(other): | ||
| # PeriodIndex + TimedeltaIndex _sometimes_ is valid |
Codecov Report
@@ Coverage Diff @@
## master #23031 +/- ##
==========================================
- Coverage 92.19% 92.14% -0.06%
==========================================
Files 169 170 +1
Lines 50911 51103 +192
==========================================
+ Hits 46939 47087 +148
- Misses 3972 4016 +44
Continue to review full report at Codecov.
|
|
Updated with a whatsnew and more tests. Doing this right required more a refactor than I expected, but in the end it touches only arithmetic code and doesn't fiddle with indexing code (like the previous attempt did) |
|
|
||
| def _add_delta(self, other): | ||
| ordinal_delta = self._maybe_convert_timedelta(other) |
There was a problem hiding this comment.
Getting the _maybe_convert_timedelta call out of the arithmetic ops makes everything simpler
| new_values = self._add_delta_tdi(other) | ||
| return self._shallow_copy(new_values) | ||
| else: # pragma: no cover | ||
| raise TypeError(type(other).__name__) |
There was a problem hiding this comment.
Looking at this and some of the other methods in the datetime/timedelta/period array mixins, I think some de-duplication can be done on the next pass
|
gentle ping |
| @@ -454,6 +480,38 @@ def _maybe_convert_timedelta(self, other): | |||
| raise IncompatibleFrequency(msg.format(cls=type(self).__name__, | |||
| freqstr=self.freqstr)) | |||
|
|
|||
| def _check_timedeltalike_freq_compat(self, other): | |||
| assert isinstance(self.freq, Tick) # checked by calling function | |||
There was a problem hiding this comment.
can u add a doc string here
| if isinstance(other, (timedelta, np.timedelta64, Tick)): | ||
| nanos = delta_to_nanoseconds(other) | ||
|
|
||
| elif isinstance(other, np.ndarray): |
There was a problem hiding this comment.
can u comments these cases a bit
|
lgtm. just add a couple of comments. merge when green |
|
Ping |
* Fix PeriodIndex +- TimedeltaIndex * better comment * typo fixup * Allow _add_delta_tdi to handle ndarray[timedelta64] gracefully * Implement specialized versions of _add_delta, _add_delta_tdi * Add tests for adding offset scalar * add tests for subtracting offset scalar * remove superfluous comment * whatsnew * Revert incorrect docstring change * docstring * comment * comment
git diff upstream/master -u -- "*.py" | flake8 --diff