Skip to content
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

API/DEPR: re-instate timedelta[non-unit] conversions as a no-op #19225

Closed
jreback opened this issue Jan 13, 2018 · 9 comments
Closed

API/DEPR: re-instate timedelta[non-unit] conversions as a no-op #19225

jreback opened this issue Jan 13, 2018 · 9 comments
Labels
API Design Closing Candidate May be closeable, needs more eyeballs Deprecate Functionality to remove in pandas Numeric Operations Arithmetic, Comparison, and Logical operations Timedelta Timedelta data type

Comments

@jreback
Copy link
Contributor

jreback commented Jan 13, 2018

when I originally added first class timedelta support I allowed .astype('m8[unit]') to return a float, except if unit==ns. IOW it was de-facto the same a dividing by Timedelta(1, unit=unit)

In [1]: s = Series(pd.to_timedelta(['1 day', '1 minutes', '1 second']))

In [2]: s
Out[2]: 
0   1 days 00:00:00
1   0 days 00:01:00
2   0 days 00:00:01
dtype: timedelta64[ns]

In [3]: s.astype('timedelta64[s]')
Out[3]: 
0    86400.0
1       60.0
2        1.0
dtype: float64

In [4]: s / pd.Timedelta('1 s')
Out[4]: 
0    86400.0
1       60.0
2        1.0
dtype: float64

In [5]: s.astype('timedelta64[ns]')
Out[5]: 
0   1 days 00:00:00
1   0 days 00:01:00
2   0 days 00:00:01
dtype: timedelta64[ns]

note that for [5], however we just return a timedelta64[ns] (which happens to be the underlying data representation).

#19224 fixes construction of non-ns (and preserves the astype freq conversions. I think that this is confusing to the user and we should revert this and have a m8[unit] just work the same as it does for M8[unit] namely that you get back a timedelta dtype (and not a float), that is the same

So [3] would be the same as [5]

We do this for datetime types now

In [13]: Series(pd.date_range('20170101', periods=3)).astype('M8[s]')
Out[13]: 
0   2017-01-01
1   2017-01-02
2   2017-01-03
dtype: datetime64[ns]

sure this is slightly convenient but a bit confusing.

I would propose that we provide a deprecation warning and change it in the next version.

@jreback jreback added Reshaping Concat, Merge/Join, Stack/Unstack, Explode API Design Timedelta Timedelta data type Deprecate Functionality to remove in pandas labels Jan 13, 2018
@jreback jreback added this to the 0.23.0 milestone Jan 13, 2018
@jreback
Copy link
Contributor Author

jreback commented Jan 13, 2018

@jreback jreback changed the title API/DEPR: re-instate timedelta[non-unit] conversions as a no-op API/DEPR: re-instate timedelta[non-unit] conversions as a no-op Jan 13, 2018
@jorisvandenbossche
Copy link
Member

jorisvandenbossche commented Jan 13, 2018

My first reaction was: and why not having the actual timedelta resolution? As we support this when doing astype("m8[s]) starting from numerical data.

But, this is actually maybe a bug / also not consistent, as in theory we only support 'ns' resolution:

In [4]: pd.Series([1,2,3]).astype('m8[s]')
Out[4]: 
0   00:00:01
1   00:00:02
2   00:00:03
dtype: timedelta64[s]    <---- kept the 's' resolution!

and for datetime it is interpreted correctly as well but then converted to ns:

In [7]: pd.Series([1,2,3]).astype('M8[s]')
Out[7]: 
0   1970-01-01 00:00:01
1   1970-01-01 00:00:02
2   1970-01-01 00:00:03
dtype: datetime64[ns]    <---- it are seconds, but dtype resolution is 'ns'

On the actual issue, I have to think a bit further about it, but agree that the float result is very strange

@jreback
Copy link
Contributor Author

jreback commented Jan 13, 2018

[4] is fixed by PR #19224 (I don't know if we had an issue about that before)

[7] is also fixed by that (its not a regression per-se because I broke it in a prior PR in master/0.23).

but note that the examples are not relevant to THIS issue. We are taking something that is already a dateimelike type and astyping it.

@jorisvandenbossche
Copy link
Member

I wanted to open a separate issue for that (as the timedelta[s] series should not be created, as once you do operations with it you get bugs by it sometimes being interpreted as ns and sometimes as s), but we already have #12425 were this is also discussed.

@jorisvandenbossche
Copy link
Member

[7] is also fixed by that (its not a regression per-se because I broke it in a prior issue).

[7] that I showed was already correct no? So what is exactly fixed/changed there?

@jreback
Copy link
Contributor Author

jreback commented Jan 13, 2018

#12425 is covered by #19224

[7] is the issue in #19223 (not broken in 0.22, rather in master)

@jreback jreback modified the milestones: 0.23.0, 0.24.0 Apr 24, 2018
@jreback jreback modified the milestones: 0.24.0, 0.25.0 Oct 23, 2018
@TomAugspurger
Copy link
Contributor

Pushing.

@TomAugspurger TomAugspurger modified the milestones: 0.25.0, 0.25.1 Jul 2, 2019
@TomAugspurger TomAugspurger modified the milestones: 0.25.1, 1.0 Aug 20, 2019
@TomAugspurger TomAugspurger modified the milestones: 1.0, Contributions Welcome Dec 30, 2019
@mroeschke mroeschke added Numeric Operations Arithmetic, Comparison, and Logical operations and removed Reshaping Concat, Merge/Join, Stack/Unstack, Explode labels May 3, 2020
@jbrockmendel
Copy link
Member

i think we're settled on a long-term solution of eventually supporting timedelta64[non-ns], so probably can close this

@jbrockmendel jbrockmendel added the Closing Candidate May be closeable, needs more eyeballs label Sep 27, 2020
@mroeschke
Copy link
Member

Yes I think we want to support this as an EA instead of simply no-oping. Closing but happy to reopen if I misunderstood

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Design Closing Candidate May be closeable, needs more eyeballs Deprecate Functionality to remove in pandas Numeric Operations Arithmetic, Comparison, and Logical operations Timedelta Timedelta data type
Projects
None yet
Development

No branches or pull requests

5 participants