DEPR: deprecate integer add/sub with DTI/TDI/PI/Timestamp/Period #22535
Conversation
|
There are some reference to integer arithmetic in |
|
Is there a usage of |
I think you can pass a list? (you can to the underlying machinery, so maybe this just works, if not it should) |
|
@jbrockmendel pls add to the list in the deprecation issue (#6581) , when we merge can check the box |
doc/source/whatsnew/v0.24.0.txt
Outdated
| @@ -514,6 +514,8 @@ Deprecations | |||
| - The signature of :meth:`Series.to_csv` has been uniformed to that of doc:meth:`DataFrame.to_csv`: the name of the first argument is now 'path_or_buf', the order of subsequent arguments has changed, the 'header' argument now defaults to True. (:issue:`19715`) | |||
| - :meth:`Categorical.from_codes` has deprecated providing float values for the ``codes`` argument. (:issue:`21767`) | |||
| - :func:`pandas.read_table` is deprecated. Instead, use :func:`pandas.read_csv` passing ``sep='\t'`` if necessary (:issue:`21948`) | |||
| - Addition or subtraction of integers with :class:`Timestamp`, :class:`Period`, :class:`DatetimeIndex`, :class:`TimedeltaIndex`, :class:`PeriodIndex` is deprecated, will be removed in a future version (:issue:`21939`) | |||
| - Addition or subtraction of integer-dtyped arrays with:class:`DatetimeIndex`, :class:`TimedeltaIndex`, :class:`PeriodIndex` is deprecated, will be removed in a future version (:issue:`21939`) | |||
pandas/_libs/tslibs/timestamps.pyx
Outdated
|
|
||
| if is_timedelta64_object(other): | ||
| other_int = other.astype('timedelta64[ns]').view('i8') | ||
| return Timestamp(self.value + other_int, | ||
| tz=self.tzinfo, freq=self.freq) | ||
|
|
||
| elif is_integer_object(other): | ||
| if self.freq is None: |
There was a problem hiding this comment.
is it necessary for freq to be none here? (we don't have this check elsewhere)
There was a problem hiding this comment.
Should be not-None, good catch.
There was a problem hiding this comment.
does this need to be changed?
jreback
added
Datetime
|
Will update as indicated. BTW gentle ping re #22350; many upcoming PRs touch those files, so it'd be helpful to get it out of the way. |
Doesn't look like it, I'll fix that. |
Trying to implement this, I've got most of it working, but can find no combination of |
|
don't worry about checking the stacklevel too much, the idea is to raise it to a reasonable level, e.g. if you run the tests, it should point to the top-level of the code where its most releveant (rather than a lower level call). Just pick something that looks reasonable, and you can turn off the stacklevel check itself. |
|
Hello @jbrockmendel! Thanks for updating the PR.
Comment last updated on October 15, 2018 at 19:17 Hours UTC |
Codecov Report
@@ Coverage Diff @@
## master #22535 +/- ##
==========================================
- Coverage 92.21% 92.21% -0.01%
==========================================
Files 161 161
Lines 51173 51191 +18
==========================================
+ Hits 47190 47206 +16
- Misses 3983 3985 +2
Continue to review full report at Codecov.
|
pandas/_libs/tslibs/timestamps.pyx
Outdated
|
|
||
| if is_timedelta64_object(other): | ||
| other_int = other.astype('timedelta64[ns]').view('i8') | ||
| return Timestamp(self.value + other_int, | ||
| tz=self.tzinfo, freq=self.freq) | ||
|
|
||
| elif is_integer_object(other): | ||
| if self.freq is None: |
There was a problem hiding this comment.
does this need to be changed?
doc/source/whatsnew/v0.24.0.txt
Outdated
| @@ -556,6 +556,8 @@ Deprecations | |||
| - :func:`pandas.read_table` is deprecated. Instead, use :func:`pandas.read_csv` passing ``sep='\t'`` if necessary (:issue:`21948`) | |||
| - :meth:`Series.str.cat` has deprecated using arbitrary list-likes *within* list-likes. A list-like container may still contain | |||
| many ``Series``, ``Index`` or 1-dimensional ``np.ndarray``, or alternatively, only scalar values. (:issue:`21950`) | |||
| - Addition or subtraction of integers with :class:`Timestamp`, :class:`Period`, :class:`DatetimeIndex`, :class:`TimedeltaIndex`, :class:`PeriodIndex` is deprecated, will be removed in a future version (:issue:`21939`) | |||
There was a problem hiding this comment.
pls make a sub-section and show what is being deprecated and what is the alternative. this is actually a rather large change.
There was a problem hiding this comment.
I'll try to get to this tomorrow.
There was a problem hiding this comment.
@jreback just added this section; wasn't sure on where in the doc to put it and the ipython/code-block syntax is a shot in the dark. Please advise.
| Current Behavior: | ||
|
|
||
| .. ipython:: python | ||
|
|
There was a problem hiding this comment.
you don't need the ipython line numbers
pandas/_libs/tslibs/period.pyx
Outdated
| @@ -1645,6 +1646,13 @@ cdef class _Period(object): | |||
| elif other is NaT: | |||
| return NaT | |||
| elif util.is_integer_object(other): | |||
| warnings.warn("Addition of integers to {cls} is " | |||
pandas/_libs/tslibs/timestamps.pyx
Outdated
|
|
||
| if is_timedelta64_object(other): | ||
| other_int = other.astype('timedelta64[ns]').view('i8') | ||
| return Timestamp(self.value + other_int, | ||
| tz=self.tzinfo, freq=self.freq) | ||
|
|
||
| elif is_integer_object(other): | ||
| if self.freq is not None: |
There was a problem hiding this comment.
so I would put the warning function in this module and just call it from higher levels
pandas/_libs/tslibs/timestamps.pyx
Outdated
| # ---------------------------------------------------------------------- | ||
|
|
||
| def int_op_deprecated(obj): |
There was a problem hiding this comment.
ok this looks good. can you rename to
``maybe_integer_op_deprecated`
|
|
||
| per = pd.Period('2016Q1') | ||
| per + 3 * per.freq | ||
|
|
There was a problem hiding this comment.
you might want to show the deprecation warning itself on 1 case here (you will need to add :okwarning: though)
pandas/core/arrays/period.py
Outdated
| @@ -778,7 +784,7 @@ def _add_delta_tdi(self, other): | |||
| assert isinstance(self.freq, Tick) # checked by calling function | |||
|
|
|||
| delta = self._check_timedeltalike_freq_compat(other) | |||
| return self._addsub_int_array(delta, operator.add).asi8 | |||
There was a problem hiding this comment.
huh? what are you adding a new kwarg for?
There was a problem hiding this comment.
We still call _addsub_int_array internally from a few places (here and in DateOffset apply_index methods) and want to suppress the warning.
But actually now that the warning is a one-liner, it is less cumbersome to put it directly in __add__/__sub__. Will change.
| @@ -1066,14 +1067,16 @@ def test_dti_iadd_int(self, tz_naive_fixture, one): | |||
| periods=10, tz=tz) | |||
| expected = pd.date_range('2000-01-01 10:00', freq='H', | |||
| periods=10, tz=tz) | |||
| rng += one | |||
| with tm.assert_produces_warning(FutureWarning, check_stacklevel=False): | |||
There was a problem hiding this comment.
why can't you check the stacklevel anymore?
There was a problem hiding this comment.
We basically never did. I never got the stacklevel checks to pass in any consistent way.
| if self.n > 0: | ||
| shifted = (i.to_perioddelta('B') - time).asi8 != 0 | ||
|
|
||
| # Integer-array addition is deprecated, so we use |
There was a problem hiding this comment.
this comment is out of date, but can change later
|
thanks @jbrockmendel sometimes they take a lot of iterations :-D |
|
It's all about the power of Teamwork. |
|
@jbrockmendel what was actually the rationale of deprecating it for Periods as well? In any case, for PeriodIndex, this was an explicitly documented behaviour, and the documentation is now no longer up to date. (and to be clear: I fully agree with the deprecation for DatetimeIndex / TimedeltaIndex!) |
I'm open to that. The ones I care about are the TS/TD/DTI/TDI |
|
After commenting here I opened an issue with the same content: #24305. Can you post it there? |
Discussed in #21939; these operations cause headaches and internal-inconsistencies (e.g. ATM
Timedeltais the only one of the related classes that doesn't support integer ops)git diff upstream/master -u -- "*.py" | flake8 --diff