Repository: pandas-dev/pandas
Pull request: BUG/API: Disallow unit if input to Timedelta and to_timedelta is/contains str
- [x] closes #12136
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


Problem statement:
BUG/API: to_timedelta unit-argument ignored for string input
So, I don't know if this is by design, but it sure confused me. `to_timedelta()` operates differently on the number `1000` compared to the string `'1000'`:

``` python
In[3]: pd.to_timedelta(1000, unit='ms')
Out[3]: Timedelta('0 days 00:00:01')
In[4]: pd.to_timedelta(1000.0, unit='ms')
Out[4]: Timedelta('0 days 00:00:01')
```

and

``` python
In[6]: pd.to_timedelta('1000ms')
Out[6]: Timedelta('0 days 00:00:01')
```

while

``` python
In[5]: pd.to_timedelta('1000', unit='ms')
Out[5]: Timedelta('0 days 00:00:00.000001')
```

I've tried my best to come up with a reason for why this behaviour would be desirable, but I can't think of any.

Hints from issue discussion:
this is a bug. The unit argument only matters when you don't have units for a particular entry.
However when parsing as a string IIRC its just ignored, so that should be handled (this is in the cython code)

separately, I think we might want an error if you have mixed with unit specifiers and unit is specified e.g.

```
In [2]: pd.to_timedelta(['1000ms',1000])
Out[2]: TimedeltaIndex(['00:00:01', '00:00:00.000001'], dtype='timedelta64[ns]', freq=None)

In [3]: pd.to_timedelta(['1000ms',1000],unit='ms')
Out[3]: TimedeltaIndex(['00:00:01', '00:00:01'], dtype='timedelta64[ns]', freq=None)
```

the first has a default of `unit='ns'`, so maybe want to make the default `None`  (and you can still
default it if its all integer like now).

want to do a pull-request? 

> want to do a pull-request?

I don't promise anything, but I can take a look at the code base and see if I can get my head around what needs to be done.

gr8. contributing docs are [here](http://pandas.pydata.org/pandas-docs/stable/contributing.html)

post any questions on this issue.

I finally got around to looking at this. I noticed @duncanwp made some commits targeting this issue. Is that enough to solve it?

@belteshassar you can certainly start with that. No pull-request yet.

No, sorry. I thought about passing the unit string into parse_timedelta_string deep down on tslib.pyx explicitly and made a bit of progress, but it's not finished. 

Feel free to pick it up or disregard as you like, I don't think I'll have time to finish it. 

While working on this, I've found another issue ~~that I'll try to fix with the same patch~~. 

Edit: Was a bit more complicated than I thought. Posted it as #12691 instead.

I think you can go ahead and add, I don't think there is a reason. Further, you can reorg the tests a bit: see [here](https://github.com/pydata/pandas/blob/master/pandas/tseries/tests/test_timedeltas.py#L760), and make several more test functions (as the current one is getting long).

The real diff for month,years is they are fixed-length units (because they are not relative to things like offsets), so while accepted are not that useful.

> separately, I think we might want an error if you have mixed with unit specifiers and unit is specified e.g.

I've been thinking a bit about this and would like some more input. As I see it, we have at least five cases that could appear:
1. `to_timedelta('1234', unit='ms')`: This one is straight forward and what initially started the issue.
2. `to_timedelta('1234ms', unit='ms')`: Here the unit matches the unit in the string, this should be discouraged, but does it really need to raise an error when there is no ambiguity?
3. `to_timedelta('1234us', unit='ms')`: Now this is becoming ambiguous. One could argue that this should raise ValueError, but on the other hand, one could decide to use the unit parameter as a suggestion and ignore it for all strings with included unit qualifiers.
4. `to_timedelta('01:02:03', unit='ms')`: Here we have a recognized format that is incompatible with the unit specifed.
5. `to_timedelta('01:02', unit='m')`: Here we have a recognized format that is possibly compatible with the unit specifed.

I lean against either raising `ValueError` for all non-numeric strings if unit is specified or simply ignoring `unit` if the string cannot be cast to float.

I would accept 1), raise on the rest.
if you have a `unit` arg, easiest is to `pd.to_numeric(x)` which will coerce to integer/float (and raise if not). Then you are done.

Commit message: Add test

Patch to review:
diff --git a/doc/source/whatsnew/v1.1.0.rst b/doc/source/whatsnew/v1.1.0.rst
index 2243790a663df..416291916890a 100644
--- a/doc/source/whatsnew/v1.1.0.rst
+++ b/doc/source/whatsnew/v1.1.0.rst
@@ -826,6 +826,7 @@ Timedelta
 - Bug in :func:`timedelta_range` that produced an extra point on a edge case (:issue:`30353`, :issue:`33498`)
 - Bug in :meth:`DataFrame.resample` that produced an extra point on a edge case (:issue:`30353`, :issue:`13022`, :issue:`33498`)
 - Bug in :meth:`DataFrame.resample` that ignored the ``loffset`` argument when dealing with timedelta (:issue:`7687`, :issue:`33498`)
+- Bug in :class:`Timedelta` and `pandas.to_timedelta` that ignored `unit`-argument for string input (:issue:`12136`)
 
 Timezones
 ^^^^^^^^^
diff --git a/pandas/_libs/tslibs/timedeltas.pyx b/pandas/_libs/tslibs/timedeltas.pyx
index a239804ea7bc2..464f6d17f0719 100644
--- a/pandas/_libs/tslibs/timedeltas.pyx
+++ b/pandas/_libs/tslibs/timedeltas.pyx
@@ -218,7 +218,7 @@ cdef convert_to_timedelta64(object ts, object unit):
 
 @cython.boundscheck(False)
 @cython.wraparound(False)
-def array_to_timedelta64(object[:] values, unit='ns', errors='raise'):
+def array_to_timedelta64(object[:] values, unit=None, errors='raise'):
     """
     Convert an ndarray to an array of timedeltas. If errors == 'coerce',
     coerce non-convertible objects to NaT. Otherwise, raise.
@@ -234,12 +234,15 @@ def array_to_timedelta64(object[:] values, unit='ns', errors='raise'):
     n = values.shape[0]
     result = np.empty(n, dtype='m8[ns]')
     iresult = result.view('i8')
+    has_string = False
 
     # Usually, we have all strings. If so, we hit the fast path.
     # If this path fails, we try conversion a different way, and
     # this is where all of the error handling will take place.
     try:
         for i in range(n):
+            if isinstance(values[i], str):
+                has_string = True
             if values[i] is NaT:
                 # we allow this check in the fast-path because NaT is a C-object
                 #  so this is an inexpensive check
@@ -247,16 +250,19 @@ def array_to_timedelta64(object[:] values, unit='ns', errors='raise'):
             else:
                 result[i] = parse_timedelta_string(values[i])
     except (TypeError, ValueError):
-        unit = parse_timedelta_unit(unit)
+        parsed_unit = parse_timedelta_unit(unit or 'ns')
         for i in range(n):
             try:
-                result[i] = convert_to_timedelta64(values[i], unit)
+                result[i] = convert_to_timedelta64(values[i], parsed_unit)
             except ValueError:
                 if errors == 'coerce':
                     result[i] = NPY_NAT
                 else:
                     raise
 
+    if has_string and unit is not None:
+        raise ValueError("unit must be un-specified if the input contains a str")
+
     return iresult.base  # .base to access underlying np.ndarray
 
 
@@ -1155,6 +1161,8 @@ class Timedelta(_Timedelta):
         elif isinstance(value, _Timedelta):
             value = value.value
         elif isinstance(value, str):
+            if unit is not None:
+                raise ValueError("unit must be un-specified if the value is a str")
             if len(value) > 0 and value[0] == 'P':
                 value = parse_iso_format_string(value)
             else:
diff --git a/pandas/core/arrays/timedeltas.py b/pandas/core/arrays/timedeltas.py
index f439f07790274..ad00eba366718 100644
--- a/pandas/core/arrays/timedeltas.py
+++ b/pandas/core/arrays/timedeltas.py
@@ -876,7 +876,7 @@ def f(x):
 # Constructor Helpers
 
 
-def sequence_to_td64ns(data, copy=False, unit="ns", errors="raise"):
+def sequence_to_td64ns(data, copy=False, unit=None, errors="raise"):
     """
     Parameters
     ----------
@@ -884,6 +884,7 @@ def sequence_to_td64ns(data, copy=False, unit="ns", errors="raise"):
     copy : bool, default False
     unit : str, default "ns"
         The timedelta unit to treat integers as multiples of.
+        Must be un-specifed if the data contains a str.
     errors : {"raise", "coerce", "ignore"}, default "raise"
         How to handle elements that cannot be converted to timedelta64[ns].
         See ``pandas.to_timedelta`` for details.
@@ -906,7 +907,8 @@ def sequence_to_td64ns(data, copy=False, unit="ns", errors="raise"):
     higher level.
     """
     inferred_freq = None
-    unit = parse_timedelta_unit(unit)
+    if unit is not None:
+        unit = parse_timedelta_unit(unit)
 
     # Unwrap whatever we have into a np.ndarray
     if not hasattr(data, "dtype"):
@@ -936,7 +938,7 @@ def sequence_to_td64ns(data, copy=False, unit="ns", errors="raise"):
         # cast the unit, multiply base/frac separately
         # to avoid precision issues from float -> int
         mask = np.isnan(data)
-        m, p = precision_from_unit(unit)
+        m, p = precision_from_unit(unit or "ns")
         base = data.astype(np.int64)
         frac = data - base
         if p:
@@ -1002,7 +1004,7 @@ def ints_to_td64ns(data, unit="ns"):
     return data, copy_made
 
 
-def objects_to_td64ns(data, unit="ns", errors="raise"):
+def objects_to_td64ns(data, unit=None, errors="raise"):
     """
     Convert a object-dtyped or string-dtyped array into an
     timedelta64[ns]-dtyped array.
@@ -1012,6 +1014,7 @@ def objects_to_td64ns(data, unit="ns", errors="raise"):
     data : ndarray or Index
     unit : str, default "ns"
         The timedelta unit to treat integers as multiples of.
+        Must be un-specified if the data contains a str.
     errors : {"raise", "coerce", "ignore"}, default "raise"
         How to handle elements that cannot be converted to timedelta64[ns].
         See ``pandas.to_timedelta`` for details.
diff --git a/pandas/core/computation/pytables.py b/pandas/core/computation/pytables.py
index 15d9987310f18..001eb1789007f 100644
--- a/pandas/core/computation/pytables.py
+++ b/pandas/core/computation/pytables.py
@@ -200,7 +200,10 @@ def stringify(value):
                 v = v.tz_convert("UTC")
             return TermValue(v, v.value, kind)
         elif kind == "timedelta64" or kind == "timedelta":
-            v = Timedelta(v, unit="s").value
+            if isinstance(v, str):
+                v = Timedelta(v).value
+            else:
+                v = Timedelta(v, unit="s").value
             return TermValue(int(v), v, kind)
         elif meta == "category":
             metadata = extract_array(self.metadata, extract_numpy=True)
diff --git a/pandas/core/tools/timedeltas.py b/pandas/core/tools/timedeltas.py
index 51b404b46f321..874e307f1f2a3 100644
--- a/pandas/core/tools/timedeltas.py
+++ b/pandas/core/tools/timedeltas.py
@@ -13,7 +13,7 @@
 from pandas.core.arrays.timedeltas import sequence_to_td64ns
 
 
-def to_timedelta(arg, unit="ns", errors="raise"):
+def to_timedelta(arg, unit=None, errors="raise"):
     """
     Convert argument to timedelta.
 
@@ -27,6 +27,7 @@ def to_timedelta(arg, unit="ns", errors="raise"):
     arg : str, timedelta, list-like or Series
         The data to be converted to timedelta.
     unit : str, default 'ns'
+        Must be un-specified if the arg is/contains a str.
         Denotes the unit of the arg. Possible values:
         ('W', 'D', 'days', 'day', 'hours', hour', 'hr', 'h',
         'm', 'minute', 'min', 'minutes', 'T', 'S', 'seconds',
@@ -76,7 +77,8 @@ def to_timedelta(arg, unit="ns", errors="raise"):
     TimedeltaIndex(['0 days', '1 days', '2 days', '3 days', '4 days'],
                    dtype='timedelta64[ns]', freq=None)
     """
-    unit = parse_timedelta_unit(unit)
+    if unit is not None:
+        unit = parse_timedelta_unit(unit)
 
     if errors not in ("ignore", "raise", "coerce"):
         raise ValueError("errors must be one of 'ignore', 'raise', or 'coerce'}")
@@ -104,6 +106,9 @@ def to_timedelta(arg, unit="ns", errors="raise"):
             "arg must be a string, timedelta, list, tuple, 1-d array, or Series"
         )
 
+    if isinstance(arg, str) and unit is not None:
+        raise ValueError("unit must be un-specified if the input is/contains a str")
+
     # ...so it must be a scalar value. Return scalar.
     return _coerce_scalar_to_timedelta_type(arg, unit=unit, errors=errors)
 
@@ -124,7 +129,7 @@ def _coerce_scalar_to_timedelta_type(r, unit="ns", errors="raise"):
     return result
 
 
-def _convert_listlike(arg, unit="ns", errors="raise", name=None):
+def _convert_listlike(arg, unit=None, errors="raise", name=None):
     """Convert a list of objects to a timedelta index object."""
     if isinstance(arg, (list, tuple)) or not hasattr(arg, "dtype"):
         # This is needed only to ensure that in the case where we end up
diff --git a/pandas/tests/scalar/timedelta/test_constructors.py b/pandas/tests/scalar/timedelta/test_constructors.py
index c58994d738562..2675c2a72bdfb 100644
--- a/pandas/tests/scalar/timedelta/test_constructors.py
+++ b/pandas/tests/scalar/timedelta/test_constructors.py
@@ -289,3 +289,17 @@ def test_timedelta_constructor_identity():
     expected = Timedelta(np.timedelta64(1, "s"))
     result = Timedelta(expected)
     assert result is expected
+
+
+@pytest.mark.parametrize(
+    "constructor, value, unit, expectation",
+    [
+        (Timedelta, "10s", "ms", (ValueError, "unit must be un-specified")),
+        (to_timedelta, "10s", "ms", (ValueError, "unit must be un-specified")),
+        (to_timedelta, ["1", 2, 3], "s", (ValueError, "unit must be un-specified")),
+    ],
+)
+def test_string_with_unit(constructor, value, unit, expectation):
+    exp, match = expectation
+    with pytest.raises(exp, match=match):
+        _ = constructor(value, unit=unit)


Code review comments:
diff_hunk: @@ -234,29 +234,35 @@ def array_to_timedelta64(object[:] values, unit='ns', errors='raise'):
     n = values.shape[0]
     result = np.empty(n, dtype='m8[ns]')
     iresult = result.view('i8')
+    found_string = False
 
     # Usually, we have all strings. If so, we hit the fast path.
     # If this path fails, we try conversion a different way, and
     # this is where all of the error handling will take place.
     try:
         for i in range(n):
+            if isinstance(values[i], str):
path: pandas/_libs/tslibs/timedeltas.pyx
text: @user1:
this should be in the clause on L251

diff_hunk: @@ -234,29 +234,35 @@ def array_to_timedelta64(object[:] values, unit='ns', errors='raise'):
     n = values.shape[0]
     result = np.empty(n, dtype='m8[ns]')
     iresult = result.view('i8')
+    found_string = False
 
     # Usually, we have all strings. If so, we hit the fast path.
     # If this path fails, we try conversion a different way, and
     # this is where all of the error handling will take place.
     try:
         for i in range(n):
+            if isinstance(values[i], str):
+                found_string = True
             if values[i] is NaT:
                 # we allow this check in the fast-path because NaT is a C-object
                 #  so this is an inexpensive check
                 iresult[i] = NPY_NAT
             else:
                 result[i] = parse_timedelta_string(values[i])
     except (TypeError, ValueError):
-        unit = parse_timedelta_unit(unit)
+        parsed_unit = parse_timedelta_unit(unit or 'ns')
         for i in range(n):
             try:
-                result[i] = convert_to_timedelta64(values[i], unit)
+                result[i] = convert_to_timedelta64(values[i], parsed_unit)
             except ValueError:
                 if errors == 'coerce':
                     result[i] = NPY_NAT
                 else:
                     raise
 
+    if found_string and unit is not None:
path: pandas/_libs/tslibs/timedeltas.pyx
text: @user1:
actually i don't think you even need to wait until the end, can you just do this in the same check on L251, eg.

```
if values[i] isNat:
   ....
else:
   if istance(values[i], str) and unit is None:
      raise.......
   result[i] = parse_....
```

@user1:
is there a test that actually hits this as well?

@author:
It wouldn't work raising inside the `try` block with `ValueError` being in `except`

@author:
The added test hits this when I checked with the repl

@author:
I noticed that this is insufficient because we might overlook elements due to exception in the loop.
Actually it's cleaner to have a separate for-loop outside for the check, without affecting performance

diff_hunk: @@ -234,29 +234,35 @@ def array_to_timedelta64(object[:] values, unit='ns', errors='raise'):
     n = values.shape[0]
     result = np.empty(n, dtype='m8[ns]')
     iresult = result.view('i8')
+    has_string = False
path: pandas/_libs/tslibs/timedeltas.pyx
text: @user1:
has_string -> found_string

diff_hunk: @@ -234,29 +234,35 @@ def array_to_timedelta64(object[:] values, unit='ns', errors='raise'):
     n = values.shape[0]
     result = np.empty(n, dtype='m8[ns]')
     iresult = result.view('i8')
+    has_string = False
 
     # Usually, we have all strings. If so, we hit the fast path.
     # If this path fails, we try conversion a different way, and
     # this is where all of the error handling will take place.
     try:
         for i in range(n):
+            if isinstance(values[i], str):
+                has_string = True
             if values[i] is NaT:
                 # we allow this check in the fast-path because NaT is a C-object
                 #  so this is an inexpensive check
                 iresult[i] = NPY_NAT
             else:
                 result[i] = parse_timedelta_string(values[i])
     except (TypeError, ValueError):
-        unit = parse_timedelta_unit(unit)
+        parsed_unit = parse_timedelta_unit(unit or 'ns')
         for i in range(n):
             try:
-                result[i] = convert_to_timedelta64(values[i], unit)
+                result[i] = convert_to_timedelta64(values[i], parsed_unit)
             except ValueError:
                 if errors == 'coerce':
                     result[i] = NPY_NAT
                 else:
                     raise
 
+    if has_string and unit is not None:
+        raise ValueError("unit must be un-specified if the input contains a str")
path: pandas/_libs/tslibs/timedeltas.pyx
text: @user1:
unit must not be specified