Accepts integer/float string with units and raises when unit is ambiguous
State: closed

I couldn't get tests to run on my machine so I will see if it passes on the CI servers.

- [x] closes #12136
- [ ] tests added / passed
- [ ] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


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

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

 - There are no PEP8 issues in the file [`pandas/tests/scalar/timedelta/test_construction.py`](https://github.com/Sup3rGeo/pandas/blob/85f04c9c44cc20b5dace8fd0301858fcb9bc57ce/pandas/tests/scalar/timedelta/test_construction.py) !

 - In the file [`pandas/util/testing.py`](https://github.com/Sup3rGeo/pandas/blob/85f04c9c44cc20b5dace8fd0301858fcb9bc57ce/pandas/util/testing.py), following                are the PEP8 issues :
> [Line 55:5](https://github.com/Sup3rGeo/pandas/blob/85f04c9c44cc20b5dace8fd0301858fcb9bc57ce/pandas/util/testing.py#L55): [E301](https://duckduckgo.com/?q=pep8%20E301) expected 1 blank line, found 0
> [Line 57:5](https://github.com/Sup3rGeo/pandas/blob/85f04c9c44cc20b5dace8fd0301858fcb9bc57ce/pandas/util/testing.py#L57): [E301](https://duckduckgo.com/?q=pep8%20E301) expected 1 blank line, found 0


 - Complete extra results for this file :

> file_to_check.py:2461:-258: W605 invalid escape sequence '\('
file_to_check.py:2461:-255: W605 invalid escape sequence '\)'
---



##### Comment last updated on October 01, 2018 at 12:55 Hours UTC
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/21384?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@a277e4a`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `n/a`.

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

```diff
@@            Coverage Diff            @@
##             master   #21384   +/-   ##
=========================================
  Coverage          ?   91.89%           
=========================================
  Files             ?      153           
  Lines             ?    49596           
  Branches          ?        0           
=========================================
  Hits              ?    45576           
  Misses            ?     4020           
  Partials          ?        0
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.29% <ø> (?)` | |
| #single | `41.86% <ø> (?)` | |


------

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

can you rebase and fixup
can you rebase and update according to comments
closing as stale, but would take if rebased / updated
@jreback sorry for abandoning this, but I will start working back again to finish this PR, especially given that everyone has spent time to review it.

Should I start another one or can I reply to the comments on this one?
@Sup3rGeo : You can keep pushing to this one.  I'll reopen.
Two points I would like to discuss:

1 - The behaviour so far would raise on `Timedelta('3.1416')` but not on `Timedelta('2000')`. Is this expected?

2 - If I raise on any string number without `unit` parameter, then I get an error in collection time. It appears something else was relying on this behaviour of considering '2000' string as nanoseconds?

```
_____________ ERROR collecting pandas/tests/util/test_hashing.py ______________
pandas\tests\util\test_hashing.py:13: in <module>
    class TestHashing(object):
pandas\tests\util\test_hashing.py:23: in TestHashing
    Series(pd.timedelta_range('2000', periods=9))])
pandas\core\indexes\timedeltas.py:801: in timedelta_range
    freq=freq, name=name, closed=closed)
pandas\core\indexes\timedeltas.py:194: in __new__
    closed=closed)
pandas\core\indexes\timedeltas.py:231: in _generate_range
    closed=closed)
pandas\core\arrays\timedeltas.py:159: in _generate_range
    start = Timedelta(start)
pandas\_libs\tslibs\timedeltas.pyx:1133: in pandas._libs.tslibs.timedeltas.Timedelta.__new__
    raise ValueError("Cannot convert float string without unit."
E   ValueError: Cannot convert float string without unit. Value: 2000 Type: <type 'str'>
!!!!!!!!!!!!!!!!!!! Interrupted: 1 errors during collection !!!!!!!!!!!!!!!!!!!
============ 6 skipped, 34363 deselected, 1 error in 73.15 seconds ============
```
> The behaviour so far would raise on Timedelta('3.1416') but not on Timedelta('2000'). Is this expected?

You mean the current behaviour in master? 
I don't think this is expected.

> 2 - If I raise on any string number without unit parameter, then I get an error in collection time. It appears something else was relying on this behaviour of considering '2000' string as nanoseconds?

Yes, we used such a construct in one of the tests (in "pandas\tests\util\test_hashing.py:23: in TestHashing"), but you can correct this in the test. 

That said, we should probably deprecate it first instead of simply changing.
@Sup3rGeo something else, there seem to be a bunch of unrelated changes in the diff. I suppose due something that went wrong in the rebase / update. 
Can you try:

```
git fetch upstream
git merge upstream/master
git push origin bugfix-string-timedelta
```

to see if that fixes it?
Hi!

> Yes, we used such a construct in one of the tests (in "pandas\tests\util\test_hashing.py:23: in TestHashing"), but you can correct this in the test.
> 
> That said, we should probably deprecate it first instead of simply changing.

Sorry but what do you mean by deprecating it instead of changing?

> Can you try:
> 
> ```
> git fetch upstream
> git merge upstream/master
> git push origin bugfix-string-timedelta
> ```
> to see if that fixes it?

Did it. Did it help? I think I screwed up with the rebase indeed. I could try starting fresh with a new branch if nothing helps.
> Did it. Did it help? I think I screwed up with the rebase indeed. I could try starting fresh with a new branch if nothing helps.

Apparently not .. I also tried to rebase, and removed the commits that were not yours. That seemed to have fixed some of the problems, but not all (there are still changes in `timedeltas.pyx` in the diff that are unrelated). 

So maybe starting from a fresh branch and copying over your changes there might be the easiest to do.

> Sorry but what do you mean by deprecating it instead of changing?

So currently `pd.Timedelta("2000")` works (the string being interpreted as an integer), so we could keep that working for now, but give the user a warning that this will change in the future to raise an error. 
See also http://pandas-docs.github.io/pandas-docs-travis/contributing.html#backwards-compatibility, although that is more about deprecating a full method/function, while here you would be deprecating only a specific behaviour (but some of the explanation still holds)
Closing in favor of #23025