Warn on ndarray[int] // timedelta by TomAugspurger · Pull Request #21036 · 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
Conversation11 Commits3 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 }})
Closes #19761.
In [2]: pd.DatetimeIndex(['1931', '1970', '2017']).view('i8') // pd.Timedelta(1, unit='s') pandas-dev/bin/ipython:1: FutureWarning: Floor division between integer array and Timedelta is deprecated. Use 'array // timedelta.value' instead.
Out[2]: array([-1230768000, 0, 1483228800])
Long-term, we'll recommend using to_epoch for the case where people are doing this to do conversion to unix epoch. But #14772 has a few design issues that will take some time to discuss. I think we should just recommend .value for now.
Closes pandas-dev#19761.
In [2]: pd.DatetimeIndex(['1931', '1970', '2017']).view('i8') // pd.Timedelta(1, unit='s')
pandas-dev/bin/ipython:1: FutureWarning: Floor division between integer array and Timedelta is
deprecated. Use 'array // timedelta.value' instead.
Out[2]: array([-1230768000, 0, 1483228800])
| elif other.dtype.kind == 'i': |
|---|
| # Backwards compatibility |
| # GH-19761 |
| msg = textwrap.dedent("""\ |
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 textwrap dedent if you put the string over multiple lines with implicit line continuation and string concatentation?
(I would personally find that a bit cleaner (also don't need to \), but no big deal)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been burned so many times by implicit string concatenation across lines that I try to always avoid it in the hope that it'll be removed in Python 4 :)
| @@ -308,7 +308,7 @@ We convert the ``DatetimeIndex`` to an ``int64`` array, then divide by the conve |
|---|
| .. ipython:: python |
| stamps.view('int64') // pd.Timedelta(1, unit='s') |
| stamps.view('int64') // pd.Timedelta(1, unit='s').value |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prob change this example to
In [11]: i
Out[11]: DatetimeIndex(['1931-01-01', '1970-01-01', '2017-01-01'], dtype='datetime64[ns]', freq=None)
In [12]: (i-pd.Timestamp(0)) // pd.Timedelta(1, unit='s')
Out[12]: Int64Index([-1230768000, 0, 1483228800], dtype='int64')
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense for now (till to_epoch is ready).
| # GH-19761 |
|---|
| msg = textwrap.dedent("""\ |
| Floor division between integer array and Timedelta is |
| deprecated. Use 'array // timedelta.value' instead. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you updated the example in the docs, maybe we should reflect this as well in the deprecation message?
The maybe tricky thing there is that not all ndarray[int] // timedelta operations are necessarily for epoch conversions. They would be the majority though...
On Tue, May 15, 2018 at 8:06 AM, Joris Van den Bossche < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pandas/_libs/tslibs/timedeltas.pyx <#21036 (comment)>: > @@ -1188,6 +1190,15 @@ class Timedelta(_Timedelta): if other.dtype.kind == 'm': # also timedelta-like return _broadcast_floordiv_td64(self.value, other, _rfloordiv) + elif other.dtype.kind == 'i': + # Backwards compatibility + # GH-19761 + msg = textwrap.dedent("""\ + Floor division between integer array and Timedelta is + deprecated. Use 'array // timedelta.value' instead. Since you updated the example in the docs, maybe we should reflect this as well in the deprecation message? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#21036 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABQHIpnZJXikD1ccI2FzlnxTxW-tMSEtks5tytLSgaJpZM4T-UP4> .
I don't know who else would be doing that :-) But it's true that it's not necessarily the case. I can do a PR to add that explanation in addition to the current one if you like.
That'd be great. Tracking down some dask issues to verify we don't have any other regressions right now, so we've got some time.
Should users also get warned on timedelta // int, which happens with timedelta % int too? Maybe the warning should pop up everytime a number is implicitly converted to a timestamp, as the current behavior does not seem very consistent with addition for example.