ERR: GH9513 NaT methods now raise ValueError #10372

kawochen · 2015-06-17T06:16:37Z

This is to close #9513. NaT methods now raise ValueError or return np.nan.

shoyer · 2015-06-17T08:01:02Z

pandas/tslib.pyx

+
+for _maybe_method_name in dir(NaTType):
+    _maybe_method = getattr(NaTType, _maybe_method_name)
+    if hasattr(_maybe_method, "__call__") \


use parentheses to group across multiple lines instead of \

jorisvandenbossche · 2015-06-17T08:49:48Z

Just a question: isn't it also a possibility to let it return np.nan ? Because now you get different results depending on if you access the date on a full index/series, or on a single element of that index/series

jreback · 2015-06-17T11:17:04Z

pandas/tseries/index.py

@@ -1389,14 +1389,16 @@ def time(self):
        """
        # can't call self.map() which tries to treat func as ufunc
        # and causes recursion warnings on python 2.6
-        return self._maybe_mask_results(_algos.arrmap_object(self.asobject.values, lambda x: x.time()))
+        return self._maybe_mask_results(_algos.arrmap_object(self.asobject.values,


so its better to actually define some of these (e.g. as we did for weekday to directly return np.nan), so .date()/.time() should simply return np.nan

+1 for nan.

kawochen · 2015-06-17T13:19:19Z

That makes sense. So leave weekday unchanged, and have date and time return nan? Unsure about toordinal though.

kawochen · 2015-06-18T14:54:00Z

@jreback tests are green.

jreback · 2015-06-18T15:29:22Z

pandas/tseries/tests/test_timeseries.py

-        self.assertTrue(np.isnan(NaT.weekday()))
+    def test_NaT_methods(self):
+        # GH 9513
+        methods_raise = ['astimezone', 'combine', 'ctime', 'dst', 'fromordinal',


hmm, I think today/now/utcnow/isoweekday maybe also should return nan

shoyer · 2015-06-18T15:54:06Z

I think I would still prefer to raise for date and time. It's one thing to return nan for methods/attributes that return numbers, but nobody expects that for a method that is supposed to return these specific types.

jorisvandenbossche · 2015-06-18T15:59:39Z

@shoyer and what with the attributes that don't return a number but a specific date/time object?

jreback · 2015-06-18T16:07:30Z

hmm, we could return NaT itself for date/time e.g.

def date(self):
    return self

In [8]: Timestamp('20130101').date()
Out[8]: datetime.date(2013, 1, 1)

In [9]: Timestamp('NaT').date()
Out[9]: NaT

The idea being is that you asked for a date/time you get a python object (as pandas does'nt have a specific date/time object specifically), but you have a missing value, so you get an object that represents a missing value (just typed to the missing value)

kawochen · 2015-06-18T21:44:01Z

That makes perfect sense for date. But time is a different type, so I think users can still be surprised by NaT.

shoyer · 2015-06-18T22:19:03Z

Technically, datetime is a subclass of date, not the other way around. It think it's better just to raise an error as opposed to guessing an appropriate sentinel value.

jreback · 2015-07-12T14:31:51Z

pandas/tseries/tests/test_timeseries.py

+
+        for method in methods_nan:
+            if hasattr(NaT, method):
+                self.assertTrue(getattr(NaT, method)() is np.nan)


use np.isnan for the comparison

kawochen · 2015-07-12T15:31:03Z

Most likely updating tonight. Any verdict on raising vs returning nan or NaT?

jreback · 2015-07-12T15:36:34Z

right

I think things that have date should be NaT eg
date,now,today

I am ok with time be Nat as well -even though it pushes the semantic boundaries

number like should be nan (eg weekday)
ordinal should prob raise

shoyer · 2015-07-12T17:07:13Z

I think things that have date should be NaT eg
date,now,today

Agreed. NaT is a datetime instance and also a date instance.

I am ok with time be Nat as well -even though it pushes the semantic boundaries

I disagree. NaT is not a time instance. We should not return it from a method which promises to return a time.

number like should be nan (eg weekday)
ordinal should prob raise

Agreed.

jreback · 2015-07-13T11:55:11Z

ok, so the suggestion is to raise for time. Ok not adverse to that.

kawochen · 2015-07-15T04:53:42Z

updated

jreback · 2015-07-15T12:07:17Z

doc/source/whatsnew/v0.17.0.txt

@@ -160,6 +160,8 @@ Other API Changes
 - Serialize metadata properties of subclasses of pandas objects (:issue:`10553`).


+- ``NaT``'s methods now either raise ``ValueError``, or return ``np.nan`` or ``NaT``; ``.weekday()`` now returns ``np.nan`` rather than ``raise``s; ``toordinal`` now ``raise``s (:issue:`9513`)


this sentence is confusing, either provide a conversion matrix or rework.

kawochen · 2015-07-16T11:26:58Z

updated

jreback · 2015-07-16T12:56:15Z

ok lgtm

@jorisvandenbossche @shoyer comments?

shoyer · 2015-07-16T15:42:41Z

pandas/tseries/tests/test_timeseries.py

+        nan_methods = ['weekday', 'isoweekday']
+
+        for method in raise_methods:
+            if hasattr(NaT, method):


How would NaT not have one of these methods? Generally it's better to be entirely explicit in tests, rather than running them conditionally.

Ah, I see your note below now. In that case, I'd slightly prefer to use if PY3 to adjust the list of tested methods but this is OK too.

shoyer · 2015-07-16T15:47:31Z

Lgtm

kawochen · 2015-07-16T18:45:49Z

pandas/tslib.pyx

+    if (callable(_maybe_method)
+        and not _maybe_method_name.startswith("_")
+        and _maybe_method_name not in
+            _implemented_methods + _nat_methods + _nan_methods):


Actually let me change this line. Having list concatenation in a loop doesn't seem like a good idea.

… NaT

kawochen · 2015-07-17T11:24:40Z

Updated again. Moved the list concatenation outside the loop.

jreback · 2015-07-17T13:09:00Z

lgtm, @shoyer if ok to you, pls merge.

shoyer · 2015-07-17T16:40:31Z

Merged via c87fa18

kawochen force-pushed the BUG-FIX-9513 branch from d1abe67 to 2958ef1 Compare June 17, 2015 06:50

shoyer reviewed Jun 17, 2015
View reviewed changes

jreback added Datetime Datetime data dtype Error Reporting Incorrect or improved errors from pandas labels Jun 17, 2015

jreback reviewed Jun 17, 2015
View reviewed changes

jreback added this to the 0.17.0 milestone Jun 17, 2015

kawochen force-pushed the BUG-FIX-9513 branch from 2958ef1 to 97df77b Compare June 18, 2015 02:37

jreback reviewed Jun 18, 2015
View reviewed changes

jreback reviewed Jul 12, 2015
View reviewed changes

kawochen force-pushed the BUG-FIX-9513 branch 5 times, most recently from 5452a47 to 0b72f59 Compare July 15, 2015 03:35

kawochen force-pushed the BUG-FIX-9513 branch from 0b72f59 to d04035e Compare July 15, 2015 03:43

jreback reviewed Jul 15, 2015
View reviewed changes

kawochen force-pushed the BUG-FIX-9513 branch from d04035e to ac8e589 Compare July 16, 2015 03:32

shoyer reviewed Jul 16, 2015
View reviewed changes

kawochen reviewed Jul 16, 2015
View reviewed changes

ERR: GH9513 NaT methods now raise ValueError, return np.nan or return…

a0d83ce

… NaT

kawochen force-pushed the BUG-FIX-9513 branch from ac8e589 to a0d83ce Compare July 17, 2015 04:01

shoyer closed this Jul 17, 2015

jreback mentioned this pull request Oct 28, 2015

NaTType does not support .time() #11453

Closed

anntzer mentioned this pull request Jan 20, 2020

time series plotting gap causes strange jump matplotlib/matplotlib#16266

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ERR: GH9513 NaT methods now raise ValueError #10372

ERR: GH9513 NaT methods now raise ValueError #10372

kawochen commented Jun 17, 2015

shoyer Jun 17, 2015

jorisvandenbossche commented Jun 17, 2015

jreback Jun 17, 2015

sinhrks Jun 17, 2015

kawochen commented Jun 17, 2015

kawochen commented Jun 18, 2015

jreback Jun 18, 2015

shoyer commented Jun 18, 2015

jorisvandenbossche commented Jun 18, 2015

jreback commented Jun 18, 2015

kawochen commented Jun 18, 2015

shoyer commented Jun 18, 2015

jreback Jul 12, 2015

kawochen commented Jul 12, 2015

jreback commented Jul 12, 2015

shoyer commented Jul 12, 2015

jreback commented Jul 13, 2015

kawochen commented Jul 15, 2015

jreback Jul 15, 2015

kawochen commented Jul 16, 2015

jreback commented Jul 16, 2015

shoyer Jul 16, 2015

shoyer Jul 16, 2015

shoyer commented Jul 16, 2015

kawochen Jul 16, 2015

kawochen commented Jul 17, 2015

jreback commented Jul 17, 2015

shoyer commented Jul 17, 2015

		@@ -160,6 +160,8 @@ Other API Changes
		- Serialize metadata properties of subclasses of pandas objects (:issue:`10553`).


		- ``NaT``'s methods now either raise ``ValueError``, or return ``np.nan`` or ``NaT``; ``.weekday()`` now returns ``np.nan`` rather than ``raise``s; ``toordinal`` now ``raise``s (:issue:`9513`)

ERR: GH9513 NaT methods now raise ValueError #10372

ERR: GH9513 NaT methods now raise ValueError #10372

Conversation

kawochen commented Jun 17, 2015

Choose a reason for hiding this comment

jorisvandenbossche commented Jun 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kawochen commented Jun 17, 2015

kawochen commented Jun 18, 2015

Choose a reason for hiding this comment

shoyer commented Jun 18, 2015

jorisvandenbossche commented Jun 18, 2015

jreback commented Jun 18, 2015

kawochen commented Jun 18, 2015

shoyer commented Jun 18, 2015

Choose a reason for hiding this comment

kawochen commented Jul 12, 2015

jreback commented Jul 12, 2015

shoyer commented Jul 12, 2015

jreback commented Jul 13, 2015

kawochen commented Jul 15, 2015

Choose a reason for hiding this comment

kawochen commented Jul 16, 2015

jreback commented Jul 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shoyer commented Jul 16, 2015

Choose a reason for hiding this comment

kawochen commented Jul 17, 2015

jreback commented Jul 17, 2015

shoyer commented Jul 17, 2015