BUG: Avoid Timedelta rounding when specifying unit and integer (#12690)
State: closed

- [x] closes #14156
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


Labels:
name: Bug
name: Timedelta
description: Timedelta data type

Comments:
Hello @mroeschke! Thanks for updating the PR.









Cheers ! There are no PEP8 issues in this Pull Request. :beers: 

##### Comment last updated on May 19, 2018 at 23:42 Hours UTC
Although I believe these rounding constants are correct, it appears there are precision issues now for larger integers.

```
# Example from the failing test

In [6]: v
Out[6]: 946688461.0005

In [7]: pd._libs.tslibs.timedeltas.cast_from_unit(v, 's')
Out[7]: 946688461000499968
``` 
Unfortunately using a 1000 multiplier didn't solve the issue, @jreback.

The issue with the example above is with floating point errors before rounding.

```
In [2]: v = 946688461.0005

In [3]: v - int(v)
Out[3]: 0.0004999637603759766

In [4]: round(v - int(v), 6) # precision in master
Out[4]: 0.0005

In [5]: round(v - int(v), 9) # precision in this PR (in order to preserve nanoseconds if specified)
Out[5]: 0.000499964
```

Would it be reasonable to cast `v` to a string in order to parse the whole number from the decimal?

```
In [8]: str(v).split('.')
Out[8]: ['946688461', '0005']
```
you cannot use strings ; rather you might look how CPython does this 
To come back to this, the question is if we should care. In the example of `v = 946688461.0005`, even in the full number (not only after splitting the fractional part), the actual precision is 946688461.00049996. So this is inherent problem of floating points even in the input, not only in our calculation.

As a user you can always solve this by eg doing `*1000` your data and specify a different `unit`.
Also complementing @jorisvandenbossche, I believe that the current rounding behavior is way, way more detrimental to users than this precision issue with the changes. 

This means that even if we care about this precision issue, it would be better to merge this changes (reflecting correctly the timedelta value for a given integer and unit) and just open another issue on the precision thing for large numbers.
I agree as well that premature rounding is less desirable than floating point errors. I could issue a warning for now for floats with high significant digits. 

>  I could issue a warning for now for floats with high significant digits.

Could we easily detect the case when the number of decimals is too high to preserve precision? In that case, a warning might be a good idea (then we can also give a hint on how to solve it)
@jorisvandenbossche after searching I don't think there's a reliable way to detect significant digits since not all floats are stored exactly.

An alternative for now is to live with the precision issue and offer a note in the docs? 
> An alternative for now is to live with the precision issue and offer a note in the docs?

Yes, that would be my preference. @jreback what's your view on this?
sure ok with simply documenting this as a known limitation. (doc-string and/or main docs)
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/19732?src=pr&el=h1) Report
> Merging [#19732](https://codecov.io/gh/pandas-dev/pandas/pull/19732?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/bc37ea2e05019a89adaa48159b220483598d1898?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.

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

```diff
@@           Coverage Diff           @@
##           master   #19732   +/-   ##
=======================================
  Coverage   91.84%   91.84%           
=======================================
  Files         153      153           
  Lines       49499    49499           
=======================================
  Hits        45460    45460           
  Misses       4039     4039
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.23% <ø> (ø)` | :arrow_up: |
| #single | `41.88% <ø> (ø)` | :arrow_up: |


------

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

Sorry for the delay on this PR.

I had to adjust some existing tests to account for floating point errors after this fix. Additionally, it appears we already have a note in the main docs about floating point errors when creating timestamps at the bottom of this section:

https://pandas.pydata.org/pandas-docs/stable/timeseries.html#epoch-timestamps

Ready for a final look and all green. 
@mroeschke to my last, is there an explicit test from the OP?
Added an explicit test from the original issue (which was from you @jreback) and paramed an existing test. 
Clarified the whatsnew entry and rounded the datetimes from the imported sas file. All green.
Thanks @jreback. I had reference the older issue that was replaced by OPs. 
thanks @mroeschke 
As this is strictly spoken an API change for those that used a float with less precision and now get higher precision with floating point errors, I would rather keep this for 0.24.0 ?