Repository: pandas-dev/pandas
Pull request: [BUG] Add is_coerce argument to func array_to_datetime_object (GH26122)
- closes #26122 
- 1 test added
- passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- whatsnew entry added


Problem statement:
Unexpected 'to_datetime' behaviour for datetime strings with different offset in 'arg', when errors='coerce' 
#### Code Sample, a copy-pastable example if possible

```python
import pandas as pd
arr = ['March 1, 2018 12:00:00+0400', 'March 1, 2018 12:00:00+0500', '20100240']
result = pd.to_datetime(arr, errors='coerce')
```
`>>> result`:
`Index(['March 1, 2018 12:00:00+0400', 'March 1, 2018 12:00:00+0500', '20100240'], dtype='object')`
#### Problem description
In `to_datetime` docstring:
`- If 'coerce', then invalid parsing will be set as NaT`

So.. it seems to me, that this is unexpected behaviour

#### Expected Output
`>>> result`:
`Index([2018-03-01 12:00:00+04:00, 2018-03-01 12:00:00+05:00, NaT], dtype='object')`

#### Output of ``pd.show_versions()``

<details>

INSTALLED VERSIONS
------------------
commit: None
python: 3.6.7.final.0
python-bits: 64
OS: Windows
OS-release: 10
machine: AMD64
processor: Intel64 Family 6 Model 142 Stepping 10, GenuineIntel
byteorder: little
LC_ALL: None
LANG: None
LOCALE: None.None

pandas: 0.24.2
pytest: 4.3.0
pip: 19.0.3
setuptools: 39.0.1
Cython: 0.29.3
numpy: 1.16.0
scipy: 1.2.1
pyarrow: None
xarray: 0.11.3
IPython: 7.3.0
sphinx: None
patsy: 0.5.1
dateutil: 2.8.0
pytz: 2018.9
blosc: None
bottleneck: None
tables: 3.4.4
numexpr: 2.6.9
feather: None
matplotlib: 3.0.3
openpyxl: 2.5.12
xlrd: 1.2.0
xlwt: 1.3.0
xlsxwriter: 1.1.5
lxml.etree: 4.3.2
bs4: 4.7.1
html5lib: 1.0.1
sqlalchemy: 1.3.1
pymysql: 0.9.3
psycopg2: 2.7.7 (dt dec pq3 ext lo64)
jinja2: 2.10
s3fs: 0.2.0
fastparquet: None
pandas_gbq: None
pandas_datareader: None
gcsfs: None

</details>

Hints from issue discussion:
Makes sense. Investigation and PRs are always welcome

Commit message: Add whatsnew note

Patch to review:
diff --git a/doc/source/whatsnew/v0.25.0.rst b/doc/source/whatsnew/v0.25.0.rst
index 3e559e771f126..d644bd381e0f5 100644
--- a/doc/source/whatsnew/v0.25.0.rst
+++ b/doc/source/whatsnew/v0.25.0.rst
@@ -273,7 +273,7 @@ Datetimelike
 - Bug in :class:`DataFrame` and :class:`Series` where timezone aware data with ``dtype='datetime64[ns]`` was not cast to naive (:issue:`25843`)
 - Improved :class:`Timestamp` type checking in various datetime functions to prevent exceptions when using a subclassed ``datetime`` (:issue:`25851`)
 - Bug in :class:`Series` and :class:`DataFrame` repr where ``np.datetime64('NaT')`` and ``np.timedelta64('NaT')`` with ``dtype=object`` would be represented as ``NaN`` (:issue:`25445`)
--
+- Bug in :meth:`to_datetime` which does not replace the invalid argument with ``NaT`` when error is set to coerce (:issue:`26122`)
 
 Timedelta
 ^^^^^^^^^
diff --git a/pandas/_libs/tslib.pyx b/pandas/_libs/tslib.pyx
index 50e3fb1c38cc7..dc377819df1bf 100644
--- a/pandas/_libs/tslib.pyx
+++ b/pandas/_libs/tslib.pyx
@@ -682,7 +682,8 @@ cpdef array_to_datetime(ndarray[object] values, str errors='raise',
         return ignore_errors_out_of_bounds_fallback(values), tz_out
 
     except TypeError:
-        return array_to_datetime_object(values, is_raise, dayfirst, yearfirst)
+        return array_to_datetime_object(values, is_raise, is_coerce,
+                                        dayfirst, yearfirst)
 
     if seen_datetime and seen_integer:
         # we have mixed datetimes & integers
@@ -697,7 +698,7 @@ cpdef array_to_datetime(ndarray[object] values, str errors='raise',
         elif is_raise:
             raise ValueError("mixed datetimes and integers in passed array")
         else:
-            return array_to_datetime_object(values, is_raise,
+            return array_to_datetime_object(values, is_raise, is_coerce,
                                             dayfirst, yearfirst)
 
     if seen_datetime_offset and not utc_convert:
@@ -709,7 +710,7 @@ cpdef array_to_datetime(ndarray[object] values, str errors='raise',
         #    (with individual dateutil.tzoffsets) are returned
         is_same_offsets = len(out_tzoffset_vals) == 1
         if not is_same_offsets:
-            return array_to_datetime_object(values, is_raise,
+            return array_to_datetime_object(values, is_raise, is_coerce,
                                             dayfirst, yearfirst)
         else:
             tz_offset = out_tzoffset_vals.pop()
@@ -757,7 +758,8 @@ cdef inline ignore_errors_out_of_bounds_fallback(ndarray[object] values):
 
 @cython.wraparound(False)
 @cython.boundscheck(False)
-cdef array_to_datetime_object(ndarray[object] values, bint is_raise,
+cdef array_to_datetime_object(ndarray[object] values,
+                              bint is_raise, bint is_coerce,
                               bint dayfirst=False, bint yearfirst=False):
     """
     Fall back function for array_to_datetime
@@ -806,6 +808,9 @@ cdef array_to_datetime_object(ndarray[object] values, bint is_raise,
                 pydatetime_to_dt64(oresult[i], &dts)
                 check_dts_bounds(&dts)
             except (ValueError, OverflowError):
+                if is_coerce:
+                    oresult[i] = NaT
+                    continue
                 if is_raise:
                     raise
                 return values, None
diff --git a/pandas/tests/indexes/datetimes/test_tools.py b/pandas/tests/indexes/datetimes/test_tools.py
index d6627f0fb8b72..e5d5b28e2f200 100644
--- a/pandas/tests/indexes/datetimes/test_tools.py
+++ b/pandas/tests/indexes/datetimes/test_tools.py
@@ -762,6 +762,18 @@ def test_iso_8601_strings_with_different_offsets(self):
                                   NaT], tz='UTC')
         tm.assert_index_equal(result, expected)
 
+        # GH 26122
+        ts_strings = ['March 1, 2018 12:00:00+0400',
+                      'March 1, 2018 12:00:00+0500',
+                      '20100240']
+        result = to_datetime(ts_strings, errors='coerce')
+        expected = Index([datetime(2018, 3, 1, 12, 0,
+                                   tzinfo=tzoffset(None, 14400)),
+                          datetime(2018, 3, 1, 12, 0,
+                                   tzinfo=tzoffset(None, 18000)),
+                          NaT])
+        tm.assert_index_equal(result, expected)
+
     def test_iso8601_strings_mixed_offsets_with_naive(self):
         # GH 24992
         result = pd.to_datetime([


Code review comments:
diff_hunk: @@ -757,7 +758,8 @@ cdef inline ignore_errors_out_of_bounds_fallback(ndarray[object] values):
 
 @cython.wraparound(False)
 @cython.boundscheck(False)
-cdef array_to_datetime_object(ndarray[object] values, bint is_raise,
+cdef array_to_datetime_object(ndarray[object] values,
path: pandas/_libs/tslib.pyx
text: @user1:
really? we already pass thru errors so why do u need yet another arg?

@user2:
Agreed. Instead of passing `is_raise`& `is_coerce` just pass down `errors` from `array_to_datetime`.  Please update the docstring of `array_to_datetime_object` after that change. 

@user3:
What exactly is the objection to using `is_raise` and `is_coerce`?

We use them all over `array_to_datetime`.  By passing through `errors`, you're essentially duplicating the logic `array_to_datetime` does to determine these flags, which you ultimately are going to use in `array_to_datetime_object`.

@user2:
That's fair. Since this is essentially a private method too changing the signature shouldn't be that big of a deal

@user1:
no, we already consistency pass around errors=, I think better here rather than having 2 args that are passed that could actually be in conflict.