Skip to content

ERR: GH9513 NaT methods now raise ValueError #10372

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions doc/source/whatsnew/v0.17.0.txt
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,16 @@ 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`` (:issue:`9513`)
=========================== ==============================================================
Behavior Methods
=========================== ==============================================================
``return np.nan`` ``weekday``, ``isoweekday``
``return NaT`` ``date``, ``now``, ``replace``, ``to_datetime``, ``today``
``return np.datetime64('NaT')`` ``to_datetime64`` (unchanged)
``raise ValueError`` All other public methods (names not beginning with underscores)
=========================== ===============================================================

.. _whatsnew_0170.deprecations:

Deprecations
Expand Down
3 changes: 3 additions & 0 deletions pandas/tests/test_index.py
Original file line number Diff line number Diff line change
Expand Up @@ -2879,6 +2879,9 @@ def test_union(self):
result = first.union(case)
self.assertTrue(tm.equalContents(result, everything))

def test_nat(self):
self.assertIs(DatetimeIndex([np.nan])[0], pd.NaT)


class TestPeriodIndex(DatetimeLike, tm.TestCase):
_holder = PeriodIndex
Expand Down
1 change: 0 additions & 1 deletion pandas/tests/test_series.py
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,6 @@ def test_valid_dt_with_missing_values(self):

# GH 8689
s = Series(date_range('20130101',periods=5,freq='D'))
s_orig = s.copy()
s.iloc[2] = pd.NaT

for attr in ['microsecond','nanosecond','second','minute','hour','day']:
Expand Down
3 changes: 2 additions & 1 deletion pandas/tseries/index.py
Original file line number Diff line number Diff line change
Expand Up @@ -1392,7 +1392,8 @@ 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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for nan.

lambda x: np.nan if x is tslib.NaT else x.time()))

@property
def date(self):
Expand Down
29 changes: 27 additions & 2 deletions pandas/tseries/tests/test_timeseries.py
Original file line number Diff line number Diff line change
Expand Up @@ -941,12 +941,34 @@ def test_nat_vector_field_access(self):
def test_nat_scalar_field_access(self):
fields = ['year', 'quarter', 'month', 'day', 'hour',
'minute', 'second', 'microsecond', 'nanosecond',
'week', 'dayofyear', 'days_in_month']
'week', 'dayofyear', 'days_in_month', 'daysinmonth',
'dayofweek']
for field in fields:
result = getattr(NaT, field)
self.assertTrue(np.isnan(result))

self.assertTrue(np.isnan(NaT.weekday()))
def test_NaT_methods(self):
# GH 9513
raise_methods = ['astimezone', 'combine', 'ctime', 'dst', 'fromordinal',
'fromtimestamp', 'isocalendar', 'isoformat',
'strftime', 'strptime',
'time', 'timestamp', 'timetuple', 'timetz',
'toordinal', 'tzname', 'utcfromtimestamp',
'utcnow', 'utcoffset', 'utctimetuple']
nat_methods = ['date', 'now', 'replace', 'to_datetime', 'today']
nan_methods = ['weekday', 'isoweekday']

for method in raise_methods:
if hasattr(NaT, method):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

self.assertRaises(ValueError, getattr(NaT, method))

for method in nan_methods:
if hasattr(NaT, method):
self.assertTrue(np.isnan(getattr(NaT, method)()))

for method in nat_methods:
if hasattr(NaT, method):
self.assertIs(getattr(NaT, method)(), NaT)

def test_to_datetime_types(self):

Expand Down Expand Up @@ -3520,6 +3542,9 @@ def check(val,unit=None,h=1,s=1,us=0):
result = Timestamp(NaT)
self.assertIs(result, NaT)

result = Timestamp('NaT')
self.assertIs(result, NaT)

def test_roundtrip(self):

# test value to string and back conversions
Expand Down
53 changes: 46 additions & 7 deletions pandas/tslib.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ from dateutil.relativedelta import relativedelta
from dateutil.parser import DEFAULTPARSER

from pytz.tzinfo import BaseTzInfo as _pytz_BaseTzInfo
from pandas.compat import parse_date, string_types, iteritems, StringIO
from pandas.compat import parse_date, string_types, iteritems, StringIO, callable

import operator
import collections
Expand Down Expand Up @@ -640,22 +640,61 @@ class NaTType(_NaT):
def __long__(self):
return NPY_NAT

def weekday(self):
return np.nan

def toordinal(self):
return -1

def __reduce__(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this an API change?

return (__nat_unpickle, (None, ))


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if there was a good reason for this originally....

fields = ['year', 'quarter', 'month', 'day', 'hour',
'minute', 'second', 'millisecond', 'microsecond', 'nanosecond',
'week', 'dayofyear', 'days_in_month', 'daysinmonth', 'dayofweek']
for field in fields:
prop = property(fget=lambda self: np.nan)
setattr(NaTType, field, prop)

# GH9513 NaT methods (except to_datetime64) to raise, return np.nan, or return NaT
# create functions that raise, for binding to NaTType
def _make_error_func(func_name):
def f(*args, **kwargs):
raise ValueError("NaTType does not support " + func_name)
f.__name__ = func_name
return f

def _make_nat_func(func_name):
def f(*args, **kwargs):
return NaT
f.__name__ = func_name
return f

def _make_nan_func(func_name):
def f(*args, **kwargs):
return np.nan
f.__name__ = func_name
return f

_nat_methods = ['date', 'now', 'replace', 'to_datetime', 'today']

_nan_methods = ['weekday', 'isoweekday']

_implemented_methods = ['to_datetime64']
_implemented_methods.extend(_nat_methods)
_implemented_methods.extend(_nan_methods)

for _method_name in _nat_methods:
# not all methods exist in all versions of Python
if hasattr(NaTType, _method_name):
setattr(NaTType, _method_name, _make_nat_func(_method_name))

for _method_name in _nan_methods:
if hasattr(NaTType, _method_name):
setattr(NaTType, _method_name, _make_nan_func(_method_name))

for _maybe_method_name in dir(NaTType):
_maybe_method = getattr(NaTType, _maybe_method_name)
if (callable(_maybe_method)
and not _maybe_method_name.startswith("_")
and _maybe_method_name not in _implemented_methods):
setattr(NaTType, _maybe_method_name, _make_error_func(_maybe_method_name))

def __nat_unpickle(*args):
# return constant defined in the module
return NaT
Expand Down