Ensure TDA.__init__ validates freq
State: closed

Users should not be able to construct invalid instances with the public constructor.

De-duplicates some code.

Labels:
name: Timedelta
description: Timedelta data type
name: Clean

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24666?src=pr&el=h1) Report
> Merging [#24666](https://codecov.io/gh/pandas-dev/pandas/pull/24666?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/ea8c9bf5167c6ae3740f56b9fe2ba60500c96ffb?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `96%`.

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

```diff
@@            Coverage Diff             @@
##           master   #24666      +/-   ##
==========================================
+ Coverage   92.38%   92.38%   +<.01%     
==========================================
  Files         166      166              
  Lines       52327    52310      -17     
==========================================
- Hits        48342    48327      -15     
+ Misses       3985     3983       -2
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.8% <96%> (-0.01%)` | :arrow_down: |
| #single | `43.06% <88%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/24666?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/indexes/timedeltas.py](https://codecov.io/gh/pandas-dev/pandas/pull/24666/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy90aW1lZGVsdGFzLnB5) | `90.25% <100%> (+0.02%)` | :arrow_up: |
| [pandas/core/arrays/timedeltas.py](https://codecov.io/gh/pandas-dev/pandas/pull/24666/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL3RpbWVkZWx0YXMucHk=) | `87.86% <95.45%> (-0.24%)` | :arrow_down: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/24666/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `88.09% <0%> (+0.09%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24666?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/24666?src=pr&el=footer). Last update [ea8c9bf...02acf9d](https://codecov.io/gh/pandas-dev/pandas/pull/24666?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Setup

```python
import numpy as np
import pandas as pd

data = np.arange(10_000, dtype='i8') * 24 * 3600 * 10**9
%timeit pd.arrays.TimedeltaArray(data)
%timeit pd.arrays.TimedeltaArray(data, freq='D')
%timeit pd.timedelta_range("1H", periods=10_000)

```

master

```
>>> %timeit pd.arrays.TimedeltaArray(data)
2.65 µs ± 61.4 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

>>> %timeit pd.arrays.TimedeltaArray(data, freq='D')
70.3 µs ± 1.7 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each)

>>> %timeit pd.timedelta_range("1H", periods=10_000)
209 µs ± 3.32 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)
```

PR (with master merged in)

```
>>> %timeit pd.arrays.TimedeltaArray(data)
19.6 µs ± 2.67 µs per loop (mean ± std. dev. of 7 runs, 100000 loops each)

>>> %timeit pd.arrays.TimedeltaArray(data, freq='D')
588 µs ± 7.03 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

>>> %timeit pd.timedelta_range("1H", periods=10_000)
135 µs ± 5.29 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each)
```

The one with `freq="D"` passed is (70us -> 588us) is expected, and can maybe be improved.

The firs one, no freq, is surprising. (2.65us -> 20us). I'll see where the time is being spent now.
~80% of `TDA.__init__` is in `_from_sequence`. For that ~75% is in `sequence_to_td64ns`, and ~10% in `_simple_new`.

Unfortunately, nothing in `sequence_to_td64ns` really stands out... The largest offender is

```
   845                                               # Convert whatever we have into timedelta64[ns] dtype
   846         1         21.0     21.0     31.8      if is_object_dtype(data) or is_string_dtype(data):
   847                                                   # no need to make a copy, need to convert if string-dtyped
   848                                                   data = objects_to_td64ns(data, unit=unit, errors=errors)
   849                                                   copy = False
 ```

It's just the cumulative effect of all the `is_<foo>_dtype` checks that's adding up.
Passing `dtype` to the `is_foo_dtype` checks helps a tad. Brings it down to `13us` (so only a 5x slowdown, instead of 8x).

```diff
diff --git a/pandas/core/arrays/timedeltas.py b/pandas/core/arrays/timedeltas.py
index d876862c9..e11d841c6 100644
--- a/pandas/core/arrays/timedeltas.py
+++ b/pandas/core/arrays/timedeltas.py
@@ -843,17 +843,18 @@ def sequence_to_td64ns(data, copy=False, unit="ns", errors="raise"):
         data = data._data
 
     # Convert whatever we have into timedelta64[ns] dtype
-    if is_object_dtype(data) or is_string_dtype(data):
+    dtype = getattr(data, 'dtype', data)
+    if is_object_dtype(dtype) or is_string_dtype(dtype):
         # no need to make a copy, need to convert if string-dtyped
         data = objects_to_td64ns(data, unit=unit, errors=errors)
         copy = False
 
-    elif is_integer_dtype(data):
+    elif is_integer_dtype(dtype):
         # treat as multiples of the given unit
         data, copy_made = ints_to_td64ns(data, unit=unit)
         copy = copy and not copy_made
 
-    elif is_float_dtype(data):
+    elif is_float_dtype(dtype):
         # treat as multiples of the given unit.  If after converting to nanos,
         #  there are fractional components left, these are truncated
         #  (i.e. NOT rounded)
@@ -863,14 +864,14 @@ def sequence_to_td64ns(data, copy=False, unit="ns", errors="raise"):
         data[mask] = iNaT
         copy = False
 
-    elif is_timedelta64_dtype(data):
+    elif is_timedelta64_dtype(dtype):
         if data.dtype != _TD_DTYPE:
             # non-nano unit
             # TODO: watch out for overflows
             data = data.astype(_TD_DTYPE)
             copy = False
 
-    elif is_datetime64_dtype(data):
+    elif is_datetime64_dtype(dtype):
         # GH#23539
         warnings.warn("Passing datetime64-dtype data to TimedeltaIndex is "
                       "deprecated, will raise a TypeError in a future "
```

I see two options

1. continue to speed up sequence_to_td64ns (possible a fast path argument, when the input is known to be an ndarary[i8] or m8[ns].
2. split out the freq inference of sequence_to_td64ns into a separate method.

Either of these can be done during the RC I think.
So my request in 
https://github.com/pandas-dev/pandas/pull/24666#pullrequestreview-190080984 is my only blocker right now.
> It's just the cumulative effect of all the is_<foo>_dtype checks that's adding up.

These are checks we do a ton across the code-base, so I'm definitely on board for optimizing them.

Two options come to mind:
- use `data.dtype.kind == 'O'`, which %timeit is telling me takes ~75ns vs ~1.25us for a is_<foo>_dtype check
- do a lib.infer_dtype call up-front

The data.dtype.kind checks seem more straightforward to me.

> continue to speed up sequence_to_td64ns (possible a fast path argument, when the input is known to be an ndarary[i8] or m8[ns].

certainly moving the i8 and m8[ns] cases to the top of the checks can short-circuit checking for other dtypes.

> split out the freq inference of sequence_to_td64ns into a separate method.

The freq inference in sequence_to_td64ns is pretty trivial.  Do you mean the freq validation in _from_sequence?

just pushed with faster dtype checks
> Hows the perf for the non-user-provided-freq case compare to master now?

`__init__` is ~2.8x slower than master, _simple_new is about 2.5x faster

setup
```
In [3]: arr = np.arange(1000)
In [4]: arr2 = arr.view('timedelta64[ns]')
```

PR
```
In [5]: %timeit pd.core.arrays.TimedeltaArray(arr)
The slowest run took 30.48 times longer than the fastest. This could mean that an intermediate result is being cached.
100000 loops, best of 3: 6.04 µs per loop

In [6]: %timeit pd.core.arrays.TimedeltaArray(arr2)
The slowest run took 14.43 times longer than the fastest. This could mean that an intermediate result is being cached.
100000 loops, best of 3: 5.47 µs per loop

In [7]: %timeit pd.core.arrays.TimedeltaArray._simple_new(arr)
The slowest run took 63.68 times longer than the fastest. This could mean that an intermediate result is being cached.
1000000 loops, best of 3: 880 ns per loop

In [8]: %timeit pd.core.arrays.TimedeltaArray._simple_new(arr2)
The slowest run took 24.56 times longer than the fastest. This could mean that an intermediate result is being cached.
1000000 loops, best of 3: 893 ns per loop
```

master
```
In [5]: %timeit pd.core.arrays.TimedeltaArray(arr)
The slowest run took 40.33 times longer than the fastest. This could mean that an intermediate result is being cached.
100000 loops, best of 3: 2.21 µs per loop

In [6]: %timeit pd.core.arrays.TimedeltaArray(arr2)
The slowest run took 11.29 times longer than the fastest. This could mean that an intermediate result is being cached.
100000 loops, best of 3: 1.94 µs per loop

In [7]: %timeit pd.core.arrays.TimedeltaArray._simple_new(arr)
The slowest run took 10.87 times longer than the fastest. This could mean that an intermediate result is being cached.
100000 loops, best of 3: 2.5 µs per loop

In [8]: %timeit pd.core.arrays.TimedeltaArray._simple_new(arr2)
The slowest run took 17.49 times longer than the fastest. This could mean that an intermediate result is being cached.
100000 loops, best of 3: 2.18 µs per loop
```
Just to be clear, my only blocker for the RC was the validation of the dtype in simple_new, which has been fixed.

My blockers for the final release are

1. `DTA/TDA.__init__`not accepts strings like `TimedeltaArray(np.array(['1H', '2H']))`.
2. The `__init__` methods be simple and don't do unnecessary work. We know that they should only accept `ndarray[m8[ns]` (or i8) or a TDA / Series / Index boxing those, so checking for additional dtypes is unnecessary.

So I'm perfectly fine with filing issues and going away.
I agree this is not a blocker
so... revert the dtype checks to use is_foo_dtype and we're good?
Fine for the RC.

On Wed, Jan 9, 2019 at 8:45 AM jbrockmendel <notifications@github.com>
wrote:

> so... revert the dtype checks to use is_foo_dtype and we're good?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/24666#issuecomment-452721058>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIi097Mbr-IidMWtASBP_sngm_r3Jks5vBgCRgaJpZM4Z0gKU>
> .
>

yep
Done
thanks
Shouldn't starting to use `sequence_to_td64` not wait until the discussion in https://github.com/pandas-dev/pandas/issues/24684 ? 

OK too late :)
(anyway, this only impact performance, not behaviour I think, so that is not a blocker for the RC)
This does change behavior. I believe that as of this PR, `TImedeltaArray.__init__` accepts strings

```python
In [3]: pd.arrays.TimedeltaArray(np.array(['1H']))
Out[3]:
<TimedeltaArray>
['01:00:00']
Length: 1, dtype: timedelta64[ns]
```

previously that would have raised a TypeError.

My vote is for raising an error, but not necessarily blocking the RC for that change.
hmm we should have the guarantees as in DTA right? that we only accept arrays of typed objects?
Yes they should be consistent.

TDA and DTA both recently started using sequcne_to_*ns in their
`__init__`s, which meant they started parsing strings.

On Wed, Jan 9, 2019 at 11:34 AM Jeff Reback <notifications@github.com>
wrote:

> hmm we should have the guarantees as in DTA right? that we only accept
> arrays of typed objects?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/24666#issuecomment-452779274>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIunCa4gkSJdhfr9nLBTNZ_K2IQtJks5vBigGgaJpZM4Z0gKU>
> .
>

> that we only accept arrays of typed objects?

right, inasmuch as it has been decided that crippling the public constructors is A Good Idea.
If you disagree, then https://github.com/pandas-dev/pandas/issues/24684 is
probably the place to discuss, though it's probably all been said at this point.

Not to shut that discussion down premature though... Apologies if it came off that way.

On Wed, Jan 9, 2019 at 12:10 PM jbrockmendel <notifications@github.com>
wrote:

> that we only accept arrays of typed objects?
>
> right, inasmuch as it has been decided that crippling the public
> constructors is A Good Idea.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/24666#issuecomment-452790833>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIvUAN3TxZV5OZDEhXHwpdYMQpk1Tks5vBjCjgaJpZM4Z0gKU>
> .
>