DEPR: deprecate integer add/sub with DTI/TDI/PI/Timestamp/Period by jbrockmendel · Pull Request #22535 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation52 Commits40 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Is there a usage of tm.assert_produces_warning to use when we expect the affected code to produce two warnings?
Is there a usage of tm.assert_produces_warning to use when we expect the affected code to produce two warnings?
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
| @@ -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`) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space after with
| 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: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it necessary for freq to be none here? (we don't have this check elsewhere)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be not-None, good catch.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this need to be changed?
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.
I think you can pass a list? (you can to the underlying machinery, so maybe this just works, if not it should)
Doesn't look like it, I'll fix that.
I think you can pass a list? (you can to the underlying machinery, so maybe this just works, if not it should)
Trying to implement this, I've got most of it working, but can find no combination of stacklevel= parameters that don't cause the "not set with correct stacklevel" assertion to fail. Who is the team expert on this?
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.
- There are no PEP8 issues in the file pandas/core/arrays/datetimelike.py !
- There are no PEP8 issues in the file pandas/core/resample.py !
- There are no PEP8 issues in the file pandas/plotting/_converter.py !
- There are no PEP8 issues in the file pandas/tests/arithmetic/test_datetime64.py !
- There are no PEP8 issues in the file pandas/tests/arithmetic/test_period.py !
- There are no PEP8 issues in the file pandas/tests/frame/test_timeseries.py !
- There are no PEP8 issues in the file pandas/tests/indexes/datetimelike.py !
- There are no PEP8 issues in the file pandas/tests/indexes/datetimes/test_arithmetic.py !
- There are no PEP8 issues in the file pandas/tests/indexes/period/test_period.py !
- There are no PEP8 issues in the file pandas/tests/indexes/timedeltas/test_arithmetic.py !
- There are no PEP8 issues in the file pandas/tests/indexing/test_partial.py !
- There are no PEP8 issues in the file pandas/tests/scalar/period/test_asfreq.py !
- There are no PEP8 issues in the file pandas/tests/scalar/period/test_period.py !
- There are no PEP8 issues in the file pandas/tests/scalar/timestamp/test_arithmetic.py !
- There are no PEP8 issues in the file pandas/tests/test_resample.py !
- There are no PEP8 issues in the file pandas/tests/tseries/offsets/test_offsets.py !
- There are no PEP8 issues in the file pandas/tseries/offsets.py !
Comment last updated on October 15, 2018 at 19:17 Hours UTC
| 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: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this need to be changed?
| @@ -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`) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls make a sub-section and show what is being deprecated and what is the alternative. this is actually a rather large change.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to get to this tomorrow.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to fix the docs and consolidate the warnings to a single function.
| Current Behavior: |
|---|
| .. ipython:: python |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't need the ipython line numbers
| @@ -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 " |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you do this
| 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: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so I would put the warning function in this module and just call it from higher levels
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small change, otherwise lgtm. ping on green.
| # ---------------------------------------------------------------------- |
|---|
| def int_op_deprecated(obj): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok this looks good. can you rename to
``maybe_integer_op_deprecated`
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
| per = pd.Period('2016Q1') |
| per + 3 * per.freq |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you might want to show the deprecation warning itself on 1 case here (you will need to add :okwarning: though)
| @@ -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 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh? what are you adding a new kwarg for?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why can't you check the stacklevel anymore?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment is out of date, but can change later
It's all about the power of Teamwork.
tm9k1 pushed a commit to tm9k1/pandas that referenced this pull request
@jbrockmendel what was actually the rationale of deprecating it for Periods as well?
The issue that is closed by this (#21939) discusses it for DatetimeIndex/TimedeltaIndex, but I think says to keep it for PeriodIndex for now.
In any case, for PeriodIndex, this was an explicitly documented behaviour, and the documentation is now no longer up to date.
But instead of updating the docs, I think we should also reconsider the deprecation. Or at least provide a clear alternative way.
(and to be clear: I fully agree with the deprecation for DatetimeIndex / TimedeltaIndex!)
I think we should also reconsider the deprecation
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?
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request