ENH: resolution inference for array_to_timedelta64 #63018
Conversation
|
|
||
| # These should not overflow! | ||
| exp = TimedeltaIndex([NaT]) | ||
| exp = TimedeltaIndex([NaT], dtype="m8[ns]") |
There was a problem hiding this comment.
What's the default unit that would be used in this case?
Still nanoseconds?
There was a problem hiding this comment.
the default in all-NaT cases is seconds (same as for datetimes). We made that choice for datetimes to ensure that in e.g.
left = pd.to_datetime(not_all_nat)
right = pd.to_datetime(all_nat)
result1 = pd.concat([left, right])
result2 = left - right
result1.unit and result2.unit should always get left.unit.
There was a problem hiding this comment.
Do we have a specific test about it (because here you are overriding the default by providing a dtype)? I had fetched the branch to test a few things, and seeing:
>>> pd.to_datetime([pd.NaT])
DatetimeIndex(['NaT'], dtype='datetime64[s]', freq=None)
>>> pd.to_timedelta([pd.NaT])
TimedeltaIndex([NaT], dtype='timedelta64[ns]', freq=None)
There was a problem hiding this comment.
good point. The TimedeltaIndex constructor has the same behavior. I'll try to track this down.
There was a problem hiding this comment.
just updated with the fix+test for this
My feeling is that this is a positive change |
2 participants
This implements resolution inference for array_to_timedelta64 matching the behavior for the Timedelta scalar. This does not change anything for strings, just np.timedelta64, pytimedelta, and pd.Timedelta objects.
One unintended side effect is that
pd.Series([np.timedelta64(1, "M")])raises instead of casting to "30 days 10:29:06". Need to give that some thought.There is some de-duplication that can be done either before merging or as follow-up.
This fixes some xfails. I'm pretty sure this also addresses some open issues, will need to track those down and add tests.