BUG: Don't error in pd.to_timedelta when errors=ignore
State: closed

Title is self-explanatory.  Closes #13613.


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

Comments:
## [Current coverage](https://codecov.io/gh/pydata/pandas/pull/13832?src=pr) is 85.28% (diff: 100%)

> Merging [#13832](https://codecov.io/gh/pydata/pandas/pull/13832?src=pr) into [master](https://codecov.io/gh/pydata/pandas/branch/master?src=pr) will increase coverage by **<.01%**

``` diff
@@             master     #13832   diff @@
==========================================
  Files           139        139          
  Lines         50211      50230    +19   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          42819      42837    +18   
- Misses         7392       7393     +1   
  Partials          0          0          
```

> Powered by [Codecov](https://codecov.io?src=pr). Last update [5f49638...dc39205](https://codecov.io/gh/pydata/pandas/compare/5f496380beb81d3986ca48cf41b893d95c227895...dc39205ffbef5200490ec6edc4bc61cb981c5b43?src=pr)

you need to add the list/Index tests as I indicated.

Further the problem is the separation of concerns here. the `errors` need to be totally handled in cython and NOT in python level. Because then you have the handling in 2 places which is  problematic. 

What are you talking about?  Please read my comments first.  I moved all of the error handling out of Cython for reasons that I outlined above.

that's not how Timestamp works - it needs to back

Again, I have no idea what you are saying.

@jreback : Any updates or replies to my comments?

@gfyoung so my issue with this is the following.

in the current code all handling of `errors=` for both Timedelta and Timestamp parsing (e.g. things like `array_to_datetime`) is almost exclusively handled in the cython path. 

The main reason of course is that by-definition `errors='coerce'` needs this (as we are in a cython loop and need to continue while processing), while `errors='ignore'` is generally a try/except around this code (it still tries to process by now returns an `object` array).

So while your approach seems ok, it now changes the way a reader has to understand what is going on (in timedelta vs timestamp parsing). So I am open to changing things, just want to make them consistent. 

Also I think its much easier to follow if its all in cython. No real reason to move it.

@jreback : The major issue with the current implementation was that there was significant coupling between parsing and error behaviour.  For example, take a look at this line <a href="https://github.com/pydata/pandas/blob/master/pandas/tslib.pyx#L2622">here</a>.  Why should `Timestamp` initialisation care about error handling?

Secondly, there was no real error handling at the Cython level (this bug attests to that), and IMO that that shouldn't be black-boxed into Cython but rather exposed at the Python level like we do with `to_numeric` for clarity.  The Cython level should be for handling the actual conversion process (the meat of the function), while the Python level should be for how we handle the behaviour of the conversions.

@gfyoung pls re-read what I wrote. you are changing code that is very similar in `array_to_datetime` / `convert_to_tsobject` in structure. That is the problem. I am happy to have you fix it, but you need to keep to the same pattern or change both (which I don't think am happy with).

> > Secondly, there was no real error handling at the Cython level (this bug attests to that), and IMO that that shouldn't be black-boxed into Cython but rather exposed at the Python level like we do with to_numeric for clarity. The Cython level should be for handling the actual conversion process (the meat of the function), while the Python level should be for how we handle the behaviour of the conversions.

not sure what you mean by this. 

I am happy to have this work, but it has to be consistent code. 

@jreback : I'm not sure why it matters so much that the format be the same here.  What's the justification for keeping it?  Here is my justification for writing it the way I did.

My point is that the conversion process should be separated into two parts: 1) The actual conversion, and 2) How we handle errors.  The current implementation of `to_datetime` couples them all together, forcing us to make clumsy calls (i.e. <a href="https://github.com/pydata/pandas/blob/master/pandas/tslib.pyx#L2622">here</a> as I mentioned above) where we have to specify error handling for no good reason at all.

In addition, the conversion process becomes littered with `try-except` blocks that arguably obfuscate what is happening at the Cython level.

The framework I have proposed here is exactly what happens in `to_numeric`, and it makes it nice and clear at the Python level what is happening when we do the conversion.  That same clarity is lacking in `to_datetime` and `to_timedelta`, so yes, my PR does change the way a reader has to understand what is going in `to_datetime` but IMO it is for the better.

@gfyoung as I said. I am happy to change. BUT it needs to be for BOTH `to_timedelta` and `to_timestamp`. changing code structure in only 1 is a no go.

@jreback : Again, I don't understand why this is such a big deal.  But if you insist...

@gfyoung I am not saying the code in `array_to_timestamp` is great. But the consistency matters so much here.

@jreback : fair enough, but for org purposes, can the refactoring of `to_datetime` be done in a follow-up?

sure, but do it on top of this one.

@jreback : Fair enough.  In that case, this should be good to merge if there are no other concerns.

@gfyoung right, but the point is I'd like to see the combined changes.

don't get me wrong I am all for stripping things out of `tslib.pyx` and moving to python where appropriate.

run an asv just to be sure in any event.

@jreback : Another PR that is rebased on this one...fair enough.

@gfyoung yes exactly. these are nice for a discrete series of changes. a bit more work, but allow one to work pretty easily.

@jreback : So trying to push error handling onto the Python level for `to_datetime` in the same way as I did for `to_timedelta` is very difficult.  The main reason is that the Cython methods used in `to_datetime` are also exposed as Python methods (`cpdef`) unlike those in `to_timedelta`.  Thus, moving the error handling out of the Cython level would be an API change.

Also, there already is error handling at the Python level for `to_datetime` (<a href="https://github.com/pydata/pandas/blob/master/pandas/tseries/tools.py#L376">here</a> and <a href="https://github.com/pydata/pandas/blob/master/pandas/tseries/tools.py#L384">here</a>).  I don't believe then that your argument about consistency of handling errors at the Cython level is valid, and given my arguments <a href="https://github.com/pydata/pandas/pull/13832#issuecomment-238449020">above</a>, any sort of refactoring of `to_datetime` is not necessary for this PR to be merged.

ok added some comments.

pls run an asv as well (not really sure we have enough asv converage though)

@jreback : Ran `asv` - I didn't see any noticeable perf drops.

Looks good to me.

Regarding the asv benchs, I don't think there are currently benchmarks that covert the coerce/ignore behaviour, so maybe nice to add some.

pls add some asvs, mainly for arrays of timedelta string parsing. This was previously in-line code, so like to see what happened.

@jreback : There are already benchmarks for arrays of timedelta string parsing.  However, I added one for bad parsing to exercise the `errors` parameter.

@jreback This is OK to merge for me. If you still have specific objections, can you reiterate them?

@gfyoung Did you see any changes in performance for the asv benchs you added?

@jorisvandenbossche 

the wrapping of the `Series` via try-except is incorrect as I have noted several times. This is a bug somewhere internally in construction. 

So I am -1 on merging until this is addressed.

@jreback Can you explain what you think the bug is in the Series constructor?

The try except block is to catch unconverted values, eg if you have `pd.to_timedelta(pd.Series(['apple']), errors='ignore')` (the original issue, this raises on master while it should return the original Series), those values will be fed into the `Series` call, like:

```
In [71]: Series(['apple'], dtype='m8[ns]')
...
ValueError: invalid literal for int() with base 10: 'apple'
```

Is it here you see a bug in the Series constructor? I think the above seems correct.
If this fails (like above, which means no conversion was made), the try-except block ensures to return the original values.

@jorisvandenbossche I have already explained this multiple times. This is not following the guarantees. `_convert_list` MUST return a valid Index. The fact that a try/except is needed for a specific case is the problem.