BUG/REF: TimedeltaIndex.__new__
State: closed

`TimedeltaIndex.__new__` type-checking is tough to decipher, and hides multiple bugs

```
>>> dti = pd.date_range('2016-01-01', periods=3)
>>> pd.TimedeltaIndex(np.array(dti))
TimedeltaIndex(['16801 days', '16802 days', '16803 days'], dtype='timedelta64[ns]', freq=None)
```

In addition to this incorrect behavior being supported, it is actually _tested_ in `tests.indexes.timedeltas.test_ops` in what I have to assume is a copy/paste mixup of some kind. 

```
>>> pd.TimedeltaIndex([0.5])
TimedeltaIndex(['0 days'], dtype='timedelta64[ns]', freq=None)
```

This PR fixes the constructor to

1) be much more linear and clear than the existing constructor
2) reject datetime64 and anything that isn't explicitly supported
3) accept floating-dtype if and only if they can be converted losslessly: `np.array([19.0])` is OK, `np.array([19.5])` is not.  `np.nan` is correctly converted to `NaT`

Other assorted small changes:
Move an incorrectly placed `# ----` section divider in scalar timedelta tests.
Change a repeated-4-times 4-liner (setting `result.freq = inferred_freq`) to a 2-linear
Move raising and returning-early cases to the top of `DatetimeIndex.__new__` and `TimedeltaIndex.__new__`

Has tests, does not have whatsnew note, pending suggestions as to which parts merit notes.

Does not significantly change the TimedeltaArray constructor; in an upcoming step we can move much of the logic there and have the TDI constructor wrap a/the TDA constructor.

Labels:
name: Bug
name: Refactor
description: Internal refactoring of code
name: Datetime
description: Datetime data dtype
name: Timedelta
description: Timedelta data type

Comments:
Hello @jbrockmendel! Thanks for submitting the PR.

 - There are no PEP8 issues in the file [`pandas/core/arrays/datetimes.py`](https://github.com/jbrockmendel/pandas/blob/1ff432b1f6927812c5fe0bfbb98b2bf594c8fd14/pandas/core/arrays/datetimes.py) !

 - There are no PEP8 issues in the file [`pandas/core/arrays/timedeltas.py`](https://github.com/jbrockmendel/pandas/blob/1ff432b1f6927812c5fe0bfbb98b2bf594c8fd14/pandas/core/arrays/timedeltas.py) !

 - There are no PEP8 issues in the file [`pandas/core/indexes/datetimes.py`](https://github.com/jbrockmendel/pandas/blob/1ff432b1f6927812c5fe0bfbb98b2bf594c8fd14/pandas/core/indexes/datetimes.py) !

 - There are no PEP8 issues in the file [`pandas/core/indexes/timedeltas.py`](https://github.com/jbrockmendel/pandas/blob/1ff432b1f6927812c5fe0bfbb98b2bf594c8fd14/pandas/core/indexes/timedeltas.py) !

 - There are no PEP8 issues in the file [`pandas/tests/arithmetic/test_timedelta64.py`](https://github.com/jbrockmendel/pandas/blob/1ff432b1f6927812c5fe0bfbb98b2bf594c8fd14/pandas/tests/arithmetic/test_timedelta64.py) !

 - There are no PEP8 issues in the file [`pandas/tests/indexes/timedeltas/test_construction.py`](https://github.com/jbrockmendel/pandas/blob/1ff432b1f6927812c5fe0bfbb98b2bf594c8fd14/pandas/tests/indexes/timedeltas/test_construction.py) !

 - There are no PEP8 issues in the file [`pandas/tests/indexes/timedeltas/test_ops.py`](https://github.com/jbrockmendel/pandas/blob/1ff432b1f6927812c5fe0bfbb98b2bf594c8fd14/pandas/tests/indexes/timedeltas/test_ops.py) !

 - There are no PEP8 issues in the file [`pandas/tests/scalar/timedelta/test_arithmetic.py`](https://github.com/jbrockmendel/pandas/blob/1ff432b1f6927812c5fe0bfbb98b2bf594c8fd14/pandas/tests/scalar/timedelta/test_arithmetic.py) !


@jbrockmendel It seems you are adding net a lot of code. Is that because some of the things were previously handled by the `to_timedelta` call?

But that said, I think some of those things (eg the float handling) should actually be fixed in `to_timedelta`, and not here, and the `TimedeltaIndex` constructor can keep making use of that.
> Does not significantly change the TimedeltaArray constructor; in an upcoming step we can move much of the logic there and have the TDI constructor wrap a/the TDA constructor.

@jbrockmendel as also mentioned in the other PR, Tom and I assumed that the consensus was to *not* do this (keep the TDA constructor simple), so if you still want to do this, please respond to the issue I linked then: https://github.com/pandas-dev/pandas/issues/23212
What's the issue with passing a datetime64[ns] array to TDI? NumPy is happy with it

```python
In [50]: np.asarray(dr).astype("timedelta64[ns]")
Out[50]:
array([1452816000000000000, 1452902400000000000, 1452988800000000000,
       1453075200000000000, 1453161600000000000, 1453248000000000000],
      dtype='timedelta64[ns]')
```

and we match that


```python
In [51]: np.asarray(dr).astype("timedelta64[ns]") == np.asarray(pd.TimedeltaIndex(np.asarray(dr)))
Out[51]: array([ True,  True,  True,  True,  True,  True])
```
@TomAugspurger you can do that but IMHO this is a TypeError
if you actually want a timedelta from a datetime the. you need to subtract a datetime
this is really preventing foot shooting
I would personally be fine with raising a TypeError, since I think this can be quite surprising behaviour (it's just using the integer values). But also fine to not deviate from numpy behaviour. 

But given it is numpy behaviour, so if we want to change it, I think we should deprecate it first.
> IMO this is a TypeError

"is a" -> "should be"?

I don't really disagree, and we *do* raise for `TimedeltaIndex(DatetimeIndex)`, so some consistency there would be nice. But I suspect that we have people relying on this, so a deprecation cycle would be needed.
ok by me to deprecate first
> this is really preventing foot shooting

I do realize my argument of "but NumPy allows it" isn't great :)

```python
In [3]: np.array([1, np.nan]).astype(int)
Out[3]: array([                   1, -9223372036854775808])
```

---

setting aside the TDI(ndarray[timedelta64]) stuff. I think the important question is whether we want

> Does not significantly change the TimedeltaArray constructor; in an upcoming step we can move much of the logic there and have the TDI constructor wrap a/the TDA constructor.

@jbrockmendel By "constructor" do you mean `TimedeltaArray.__init__`, or just a generic term for creating a TimedeltaArray from some input?
@jorisvandenbossche 

> But that said, I think some of those things (eg the float handling) should actually be fixed in to_timedelta, and not here,

It can be difficult to tell exactly what is handled within the to_timedelta calls because there are two of them reached under separate conditions, and most of the relevant logic is then done again below that anyway.  I'd be +1 on improving to_timedelta later.

> as also mentioned in the other PR, Tom and I assumed that the consensus was to not do this (keep the TDA constructor simple), so if you still want to do this, please respond to the issue I linked then: #23212

I'll restate my opinion there when I'm ready, thank you.  I avoided changing the Array constructors specifically to avoid this being an issue; really shouldn't have mentioned it all in the OP.
> I think the important question is whether we want [...] By "constructor" do you mean TimedeltaArray.__init__, or just a generic term for creating a TimedeltaArray from some input?

@TomAugspurger same as above; that is _an_ important question, but I went out of my way to keep it totally orthogonal to this PR.

It sounds like the discussion w/r/t `TimedeltaIndex(np.array(dti))` has resolved itself overnight with an agreement that there should be a deprecation before a TypeError.  Am I reading the room correctly?
> It sounds like the discussion w/r/t TimedeltaIndex(np.array(dti)) has resolved itself overnight with an agreement that there should be a deprecation before a TypeError. Am I reading the room correctly?

I think so.

> I'll restate my opinion there when I'm ready, thank you. I avoided changing the Array constructors specifically to avoid this being an issue; really

Ok. I think I saw "refactor `TimedeltaIndex.__new__`" and thought it meant something closer to what we did for `PeriodIndex.__new__`, but we may not be there yet.
> It can be difficult to tell exactly what is handled within the to_timedelta calls because there are two of them reached under separate conditions, and most of the relevant logic is then done again below that anyway. I'd be +1 on improving to_timedelta later.

The bug you are mentioning (`pd.TimedeltaIndex([0.5])` ignoring the decimal) is coming from `to_timedelta` doing that (I just verified it is indeed taking this path). So you are repeating and fixing here what `to_timedelta` is already doing, so if you want to fix this behaviour, it really seems logical to me to fix this in `to_timedelta` instead of expanding the code here.

> It sounds like the discussion w/r/t TimedeltaIndex(np.array(dti)) has resolved itself overnight with an agreement that there should be a deprecation before a TypeError. Am I reading the room correctly?

Yes, I think so.


> So you are repeating and fixing here what to_timedelta is already doing, so if you want to fix this behaviour, it really seems logical to me to fix this in to_timedelta instead of expanding the code here.

AFAICT the to_timedelta call (L168 in master) is _almost_ superfluous given what comes below (which was recently moved there IIRC because it was bizarrely located in `_simple_new` before that).  If we disable the branch that calls `to_timedelta`, the only other changes we need to make everything work (or specifically, pass existing tests) is to a) convert generators to lists before passing into `data = np.array(data, copy=False)` and b) catch string-dtypes in the same block that catches `np.object_` just below that.  Which is to say: calling `to_timedelta` at all is highly redundant.

That said, an argument that I _would_ find compelling is that we definitely don't want the behavior of `TimedeltaIndex([x, y, z])` to diverge from the behavior of `to_timedelta([x, y, z])`, which this PR currently does.  So I'll take a look at combining these after some more caffeine.
>  If we disable the branch that calls to_timedelta, the only other changes we need to make everything work (or specifically, pass existing tests) is to a) convert generators to lists before passing into data = np.array(data, copy=False) and b) catch string-dtypes in the same block that catches np.object_ just below that. Which is to say: calling to_timedelta at all is highly redundant.

If you are going to look at it, I would recommend trying to make the checks you mention here redundant, by letting `to_timedelta` handle it. Eg strings is something that `to_timedelta` is made for to handle. 
I think the object dtype handling could be removed, see below (but certainly didn't check all cases, so this might be too optimistic)


Dived into the (current master) code for a moment, and you have this:

https://github.com/pandas-dev/pandas/blob/28a42da41ca8e13efaa2ceb3939e576d08c232c8/pandas/core/indexes/timedeltas.py#L165-L173

Here, the conversion of everything that is not yet a timedelta64 array is first handled with `to_timedelta`. But at the end of the snippet, we still call `array_to_timedelta64(data)` (what is used inside `to_timedelta`) if the data is object dtype. But didn't we already just convert everything not m8 with `to_timedelta`? 
So this happens if `data` is a TimedeltaArrayMixin (or at least one of the cases): TimedeltaArrayMixin still has a m8[ns] numpy dtype, so the `getattr(data, 'dtype', None) != _TD_DTYPE` is passed; so we then convert it with `np.array(data)` to an array of Timedelta objects, to then convert it back to m8 with `array_to_timedelta64`. 
So this is of course just a redundant back and forth conversion, that can be avoided with a check if `data` is a TimedeltaArrayMixin, and in that case get the underlying numpy array of it. 
(similar to what DatetimeIndex also does, it has a check `if isinstance(data, DatetimeArrayMixin)`)
@jorisvandenbossche I think we're in agreement on the relevant part which is that the status quo is redundant and unclear.  Give me some time to adjust the PR to see if we can reach something closer to optimal.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/23539?src=pr&el=h1) Report
> Merging [#23539](https://codecov.io/gh/pandas-dev/pandas/pull/23539?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/58a59bd606911f11a4f679df18a00cfc25536c30?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `93.18%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/23539/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/23539?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #23539      +/-   ##
==========================================
+ Coverage   92.23%   92.23%   +<.01%     
==========================================
  Files         161      161              
  Lines       51288    51322      +34     
==========================================
+ Hits        47304    47337      +33     
- Misses       3984     3985       +1
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.62% <93.18%> (ø)` | :arrow_up: |
| #single | `42.3% <69.31%> (+0.03%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/23539?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/tools/timedeltas.py](https://codecov.io/gh/pandas-dev/pandas/pull/23539/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvdG9vbHMvdGltZWRlbHRhcy5weQ==) | `98.03% <100%> (-0.15%)` | :arrow_down: |
| [pandas/core/arrays/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/23539/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2RhdGV0aW1lcy5weQ==) | `98.44% <100%> (-0.01%)` | :arrow_down: |
| [pandas/core/indexes/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/23539/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZXMucHk=) | `96.12% <100%> (-0.02%)` | :arrow_down: |
| [pandas/core/indexes/timedeltas.py](https://codecov.io/gh/pandas-dev/pandas/pull/23539/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy90aW1lZGVsdGFzLnB5) | `89.29% <78.57%> (-0.59%)` | :arrow_down: |
| [pandas/core/arrays/timedeltas.py](https://codecov.io/gh/pandas-dev/pandas/pull/23539/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL3RpbWVkZWx0YXMucHk=) | `95.08% <94.73%> (+1.29%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/23539?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/23539?src=pr&el=footer). Last update [58a59bd...898444f](https://codecov.io/gh/pandas-dev/pandas/pull/23539?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Pretty major set of changes since the previous round:

- instead of doing all the inference/conversion inside `TimedeltaIndex.__new__`, it now occurs in `arrays.timedeltas.sequence_to_td64ns`
- `to_timedelta` now dispatches to `sequence_to_td64ns`
- more careful checks for at-most-one copy
- fastpath freq inference/validation (and tests for each new path that produces)
- passing datetime64 data is deprecated instead of raising
> Regarding the floats, I am wondering if we should also first deprecate this? As this might also be a hard break for people using it.

Actually, I think we need to revert it, as it is actually a feature. It does work as expected with bigger units (we just ignore anything smaller as a nanosecond, which is the full decimal at nanosecond resolution, but not at higher resolution):

```
In [12]: pd.TimedeltaIndex([1.5, 2.5], unit='D')
Out[12]: TimedeltaIndex(['1 days 12:00:00', '2 days 12:00:00'], dtype='timedelta64[ns]', freq=None)
```

This is broken on this branch, which also means that it is actually not tested ..
You make a good point on the floats.  I'll revert and add a test for the example you gave.

w/r/t tools.timedeltas, I think that is a distinctly poor place for this logic to live.  The logic for how we create TimedeltaArrays belongs with... TimedeltaArrays.  Especially w/r/t keeping a DAG-like dependency structure, this simplifies things.
> Does changing this require changing existing tests?

No, the error message is unchanged.
Thoughts here?  I'd like to get a move on the DTA analogue of this PR.
@gfyoung if you have any further comments.
@jbrockmendel ok let's merge this on green. a couple of minor followups (they are unresolved conversations).
thanks @jbrockmendel if you'd apply @datapythonista comments next pass would be great. trying to not block your followups.
@datapythonista I'm adding suggested edits into #23587 (needs rebasing anyway).  Definitely good suggestions.

For the "unit" description the best I've come up with is "The timedelta unit to treat integers as multiples of"; I can't think of a natural-sounding way to rephrase this that doesn't end in a preposition.  Thoughts?
That description sounds good to me. Can't think of anything better right now.