API: to_datetime, required unit with numerical (#15836) #15896

atbd · 2017-04-04T22:07:47Z

add test_to_datetime_numerical_input
check arg for numerical type

closes API: to_datetime with ordinals and no unit #15836
tests added / passed
passes git diff upstream/master --name-only -- '*.py' | flake8 --diff
whatsnew entry

jreback · 2017-04-04T22:14:09Z

pandas/tseries/tools.py

@@ -437,6 +437,10 @@ def _convert_listlike(arg, box, format, name=None, tz=tz):
    if arg is None:
        return None

+    check_arg = np.array(arg) if np.iterable(arg) else np.array([arg])


do

if unit is None: ....

move the actual is_numeric_dtype / is_scalar check (same one that is in origin) to a function (that you call in both places)

jreback · 2017-04-04T22:15:57Z

pandas/tests/indexes/datetimes/test_tools.py

@@ -715,6 +715,13 @@ def test_to_datetime_unprocessable_input(self):
        )
        self.assertRaises(TypeError, to_datetime, [1, '1'], errors='raise')

+    def test_to_datetime_numerical_input(self):
+        self.assertRaises(ValueError, to_datetime, arg=int(1000))


use with pytest.raises(...):

you can also do this with parametrize: http://pandas-docs.github.io/pandas-docs-travis/contributing.html#how-to-use-parametrize

atbd · 2017-04-05T12:27:48Z

I took into account what you said, but I still have issues with two tests test_dataframe_dtypes and test_dataframe which do not need unit to work.
The units are directly in the keys of the structures.

What behaviour to adopt in those cases ?

jreback · 2017-04-05T12:51:38Z

pandas/tests/indexes/datetimes/test_tools.py

-            to_datetime(False)
-        self.assertTrue(to_datetime(False, errors="coerce") is NaT)
-        self.assertEqual(to_datetime(False, errors="ignore"), False)
+            to_datetime(False, unit='ns')


ideally you would parametrize this on units to test all the units, see https://github.com/pandas-dev/pandas/blob/master/pandas/tests/indexes/datetimes/test_tools.py#L1520

(move the units fixture to the top of the file)

jreback · 2017-04-05T12:52:44Z

pandas/tests/indexes/datetimes/test_tools.py

@@ -715,6 +716,14 @@ def test_to_datetime_unprocessable_input(self):
        )
        self.assertRaises(TypeError, to_datetime, [1, '1'], errors='raise')

+    @pytest.mark.parametrize('arg', [int(1), float(1), range(5),


move this to a separate class that inherits from object (to avoid using staticmethod).

alternatively. We can simply remove the inheritance from tm.TestCase from this class. It was prob just copy-pasted, and prob not needed. (this is what causes parametrize to not work).

you would need to change most self.assert* methods to use tm.assert* methods.

jreback · 2017-04-05T12:53:13Z

pandas/tseries/tools.py

@@ -434,9 +434,17 @@ def _convert_listlike(arg, box, format, name=None, tz=tz):
            except (ValueError, TypeError):
                raise e

+    def check_numerical_arg():
+        return ((is_scalar(arg) and (is_integer(arg) or is_float(arg))) or


so need to exclude DataFrame from this check I think.

jreback · 2017-04-05T12:55:29Z

pandas/tseries/tools.py

    if arg is None:
        return None

+    if unit is None and format is None:


alternatively:

if unit is not None: if isinstance(arg, DataFrame): raise nice message here elif format is None and check_numerical_arg(): .....

add a specific test for passing a DataFrame with a not-None unit as well.

jorisvandenbossche · 2017-04-06T08:12:34Z

Given the changes that we have to make in our own test suite, we should maybe consider to make this a deprecation first before actually changing? (raising a warning you should provide unit, and in future not providing will raise?)

jreback · 2017-04-06T13:33:53Z

I think its better to actually make this an error now. If you are doing this would almost always don't mean ns, so that is the only case that is really affected. Otherwise its an error (not so obvious though), so better to fail this. IOW, this is a bug, not an API change.

jreback · 2017-05-13T21:40:36Z

can you rebase

codecov · 2017-07-12T12:33:44Z

Codecov Report

Merging #15896 into master will increase coverage by 0.13%.
The diff coverage is 90%.

@@            Coverage Diff             @@
##           master   #15896      +/-   ##
==========================================
+ Coverage   90.96%    91.1%   +0.13%     
==========================================
  Files         161      161              
  Lines       49301    49307       +6     
==========================================
+ Hits        44849    44923      +74     
+ Misses       4452     4384      -68

Flag	Coverage Δ
#multiple	`88.88% <90%> (+0.15%)`	⬆️
#single	`40.2% <60%> (-0.06%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/indexes/datetimes.py	`95.33% <100%> (ø)`	⬆️
pandas/core/dtypes/cast.py	`86.89% <100%> (ø)`	⬆️
pandas/core/tools/datetimes.py	`85.2% <83.33%> (+18.26%)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.71% <0%> (-0.1%)`	⬇️
pandas/plotting/_converter.py	`65.05% <0%> (+1.81%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a9421af...904e45c. Read the comment docs.

codecov · 2017-07-12T12:34:03Z

Codecov Report

Merging #15896 into master will increase coverage by 0.09%.
The diff coverage is 90%.

@@            Coverage Diff             @@
##           master   #15896      +/-   ##
==========================================
+ Coverage   91.01%    91.1%   +0.09%     
==========================================
  Files         163      161       -2     
  Lines       49570    49307     -263     
==========================================
- Hits        45116    44923     -193     
+ Misses       4454     4384      -70

Flag	Coverage Δ
#multiple	`88.88% <90%> (+0.1%)`	⬆️
#single	`40.2% <60%> (-0.12%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/indexes/datetimes.py	`95.33% <100%> (ø)`	⬆️
pandas/core/dtypes/cast.py	`86.89% <100%> (-0.93%)`	⬇️
pandas/core/tools/datetimes.py	`85.2% <83.33%> (+18.17%)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/indexes/accessors.py	`88.09% <0%> (-1.16%)`	⬇️
pandas/core/internals.py	`93.43% <0%> (-0.87%)`	⬇️
pandas/util/_decorators.py	`65.3% <0%> (-0.7%)`	⬇️
pandas/plotting/_timeseries.py	`60.2% <0%> (-0.53%)`	⬇️
pandas/core/sparse/frame.py	`94.26% <0%> (-0.45%)`	⬇️
... and 66 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77bfe21...3a7b1c0. Read the comment docs.

jreback · 2017-07-12T13:27:30Z

doc/source/whatsnew/v0.21.0.txt

@@ -40,6 +40,7 @@ Other Enhancements
 - :func:`DataFrame.clip()` and :func:`Series.clip()` have gained an ``inplace`` argument. (:issue:`15388`)
 - :func:`crosstab` has gained a ``margins_name`` parameter to define the name of the row / column that will contain the totals when ``margins=True``. (:issue:`15972`)
 - :func:`Dataframe.select_dtypes` now accepts scalar values for include/exclude as well as list-like. (:issue:`16855`)
+- :func:`to_datetime` requires an unit with numerical arg (:issue:`15836`)


this is in API breaking changes. say that it will now raise if a numeric arg is provided w/o a unit.

also needs a mini-sub-section as this is a non-obvious change. show the previous and new behavior.

jreback · 2017-07-12T13:28:29Z

pandas/_libs/tslib.pyx

@@ -2230,6 +2230,11 @@ cpdef array_with_unit_to_datetime(ndarray values, unit, errors='coerce'):
    m = cast_from_unit(None, unit)

    if is_raise:
+        from pandas.core.dtypes.common import is_bool_dtype
+
+        if is_bool_dtype(values):


I would raise on non-numeric instead here

If I raise only on non-numeric, boolean values go through and do not raise error. Plus, it causes issues with list.

ok, instead of using is_bool_dtype here, can do issubclass(isvalues.dtype, np.bool_) is slightly more idiomatic here.

I use np.issubdtype(values.dtype, np.bool_) instead of is_bool_dtype

jreback · 2017-07-12T13:29:22Z

pandas/core/tools/datetimes.py

@@ -234,7 +234,7 @@ def to_datetime(arg, errors='raise', dayfirst=False, yearfirst=False,
        - If True, require an exact format match.
        - If False, allow the format to match anywhere in the target string.

-    unit : string, default 'ns'
+    unit : string
        unit of the arg (D,s,ms,us,ns) denote the unit, which is an
        integer or float number. This will be based off the origin.


add to the raises section in doc-string that a ValueError on non-numeric input when a unit is specified

this is default to None

jreback · 2017-07-12T13:30:28Z

pandas/core/tools/datetimes.py

@@ -446,8 +447,15 @@ def _convert_listlike(arg, box, format, name=None, tz=tz):
            except (ValueError, TypeError):
                raise e

+    def check_numerical_arg():
+        return ((is_scalar(arg) and (is_integer(arg) or is_float(arg))) or


why are you adding the .size check?

Without it, pd.to_datetime([]) raises a ValueError("a unit is required in case of numerical arg") which is not the actual behaviour in some others tests (pandas/tests/test_resample.py::TestPeriodIndex::test_resample_empty_dtypes for example).

ok, then just do len(arg) instead (its by-definition already an interable here)

It can be a scalar argument too (example: pd.to_datetime(2, unit='ns') ; arg = 2 here), i have to do a np.asarray

no it can't that would hit the first clause (the is_scalar)

You're right, wrong example. It is bool element that go wrong.

In [8]: pd.to_datetime(True, unit='ns') TypeError: object of type 'bool' has no len()

jreback · 2017-08-17T10:38:35Z

can you rebase and update to comments.

All numerical types now need a unit in to_datetime call. * Update tests * Add bool verification in tslib.pyx * Bug correction with bool and unit in ['ms', 'us', 's', 'D'] * to_datetime(True, unit='ms') did run without error * to_datetime(True, unit='ms', errors='ignore') returned None Issue: pandas-dev#15836

jreback · 2017-10-28T00:29:36Z

can you rebase and update

jreback · 2017-11-12T19:49:37Z

closing as stale. if you'd like to continue pls ping.

jreback reviewed Apr 4, 2017

View reviewed changes

atbd force-pushed the 15836 branch from 81b3008 to 8e56980 Compare April 5, 2017 12:21

jreback reviewed Apr 5, 2017

View reviewed changes

atbd force-pushed the 15836 branch from 8e56980 to c4c8265 Compare April 6, 2017 08:55

jreback added API Design Bug Datetime Datetime data dtype labels Apr 6, 2017

atbd force-pushed the 15836 branch 2 times, most recently from f723be4 to 0520b70 Compare April 25, 2017 13:52

atbd changed the title ~~[WIP] API: to_datetime, required unit with numerical (#15836)~~ API: to_datetime, required unit with numerical (#15836) Apr 25, 2017

atbd force-pushed the 15836 branch from 0520b70 to f01732a Compare April 26, 2017 15:21

atbd force-pushed the 15836 branch from f01732a to e569a0f Compare May 15, 2017 14:27

jorisvandenbossche mentioned this pull request Jun 28, 2017

to_timedelta('1', unit='h') does not work. #16791

Closed

atbd force-pushed the 15836 branch 2 times, most recently from a411af6 to 904e45c Compare July 12, 2017 12:33

atbd force-pushed the 15836 branch from 904e45c to d8eff82 Compare July 12, 2017 13:14

jreback requested changes Jul 12, 2017

View reviewed changes

atbd force-pushed the 15836 branch from d8eff82 to 3a7b1c0 Compare August 30, 2017 13:55

jreback closed this Nov 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API: to_datetime, required unit with numerical (#15836) #15896

API: to_datetime, required unit with numerical (#15836) #15896

atbd commented Apr 4, 2017 •

edited

Loading

jreback Apr 4, 2017

jreback Apr 4, 2017

jreback Apr 4, 2017

atbd commented Apr 5, 2017 •

edited

Loading

jreback Apr 5, 2017

jreback Apr 5, 2017

jreback Apr 5, 2017

jreback Apr 5, 2017

jreback Apr 5, 2017

jreback Apr 5, 2017

jorisvandenbossche commented Apr 6, 2017

jreback commented Apr 6, 2017

jreback commented May 13, 2017

codecov bot commented Jul 12, 2017

codecov bot commented Jul 12, 2017 •

edited

Loading

jreback Jul 12, 2017

jreback Jul 12, 2017

jreback Jul 12, 2017

atbd Jul 12, 2017 •

edited

Loading

jreback Jul 12, 2017

atbd Jul 13, 2017

jreback Jul 12, 2017

jreback Jul 12, 2017

jreback Jul 12, 2017

atbd Jul 12, 2017

jreback Jul 12, 2017

atbd Jul 13, 2017

jreback Jul 13, 2017

atbd Jul 14, 2017

jreback commented Aug 17, 2017

jreback commented Oct 28, 2017

jreback commented Nov 12, 2017

API: to_datetime, required unit with numerical (#15836) #15896

API: to_datetime, required unit with numerical (#15836) #15896

Conversation

atbd commented Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atbd commented Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisvandenbossche commented Apr 6, 2017

jreback commented Apr 6, 2017

jreback commented May 13, 2017

codecov bot commented Jul 12, 2017

Codecov Report

codecov bot commented Jul 12, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atbd Jul 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Aug 17, 2017

jreback commented Oct 28, 2017

jreback commented Nov 12, 2017

atbd commented Apr 4, 2017 •

edited

Loading

atbd commented Apr 5, 2017 •

edited

Loading

codecov bot commented Jul 12, 2017 •

edited

Loading

atbd Jul 12, 2017 •

edited

Loading