BUG/API: to_timedelta unit-argument ignored for string input

[belteshassar]

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':

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

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

while

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.

[jreback]

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?

[belteshassar]

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.

[jreback]

gr8. contributing docs are here

post any questions on this issue.

[belteshassar]

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

[jreback]

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

[duncanwp]

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.

[belteshassar]

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.

[jreback]

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, 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.