REGR: to_timedelta precision issues with floating data
State: closed

Closes https://github.com/pandas-dev/pandas/issues/25077

Labels:
name: Regression
description: Functionality that used to work in a prior pandas version
name: Timedelta
description: Timedelta data type

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

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

```diff
@@            Coverage Diff             @@
##           master   #25651      +/-   ##
==========================================
- Coverage   91.26%   91.26%   -0.01%     
==========================================
  Files         173      173              
  Lines       52968    52965       -3     
==========================================
- Hits        48340    48337       -3     
  Misses       4628     4628
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `89.83% <100%> (-0.01%)` | :arrow_down: |
| #single | `41.7% <100%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25651?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/arrays/timedeltas.py](https://codecov.io/gh/pandas-dev/pandas/pull/25651/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL3RpbWVkZWx0YXMucHk=) | `88.13% <100%> (-0.07%)` | :arrow_down: |

------

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

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

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

```diff
@@            Coverage Diff             @@
##           master   #25651      +/-   ##
==========================================
+ Coverage   91.29%   91.29%   +<.01%     
==========================================
  Files         173      173              
  Lines       52961    52965       +4     
==========================================
+ Hits        48349    48354       +5     
+ Misses       4612     4611       -1
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `89.87% <100%> (ø)` | :arrow_up: |
| #single | `41.73% <83.33%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25651?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/arrays/timedeltas.py](https://codecov.io/gh/pandas-dev/pandas/pull/25651/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL3RpbWVkZWx0YXMucHk=) | `88.29% <100%> (+0.09%)` | :arrow_up: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/25651/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `89.08% <0%> (+0.09%)` | :arrow_up: |

------

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

So this has a performance impact for sure, and a quite big one apparently (around 300-400 x).

> so you are causing a rather large perf regression by changing this

Compared to 0.23.x and before, there is no performance regression. But let me look if the logic is easy to port to float branch of `sequence_to_td64ns`. 
(and I doubt this change was originally done with performance in mind, in any case that is not mentioned in the PR https://github.com/pandas-dev/pandas/pull/23539)

> pls shown asv before / after

We don't have any benchmarks passing floating data to `to_timedelta` / `TimedeltaIndex`
@jreback in the last commit (https://github.com/pandas-dev/pandas/pull/25651/commits/39b15aad475b016576a655dedbb445c3690ea23c), you can see what it would look like when using the same float-handling logic from timedeltas.pyx in `sequence_to_td64ns`. 

This makes that the conversion is much faster compared to the old (pre 0.24.0) path, but should have the same result:

```
In [1]: arr = np.arange(0, 1, 1e-6)

In [9]: %timeit pd.core.arrays.timedeltas.sequence_to_td64ns(arr, unit='s')[0] 
21.8 ms ± 200 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)

In [10]: %timeit pd.core.arrays.timedeltas.objects_to_td64ns(arr, unit='s')  <-- basically what was being used < 0.24
2.84 s ± 35.3 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
```

On master, the `sequence_to_td64ns` takes only around 8 ms, so this PR would still be a 2-3 times slowdown compared to master, but still give around 100 x speedup compared to < 0.24.0.

The only thing I didn't do yet is a proper way for writing `precision_from_unit`. Currently, it is basically a copy paste from the cdef inline function `cast_from_unit` below it. But this is not usable from python, and making it non-inline might have performance implications.
cc @jbrockmendel 
>  can u just run some quick timings to see if anything else is affected by this - it’s a heavily used routine

I did some quick timings of `pd._libs.tslibs.timedeltas.array_to_timedelta64(arr, unit='s')` (with an all float array, the extreme case, which in practice will not go through this path any more) and of `pd.Timedelta("4 days 3 hours 2 minutes 1 second")`, and for those, I don't see a significant difference.
ok then - lgtm thanks 
I also just ran the timedelta benchmarks for this branch compared to master, and it says nothing changed significantly (although I don't know if we are benchmarking a critical path that relies heavily on the changed function)