BUG: Raise when casting NaT to int #28492

dsaxton · 2019-09-18T03:17:27Z

tests added / passed
passes black pandas
whatsnew entry

Fixes a bug in astype_nansafe where NaT was ignored when casting a datetime or timedelta to int. I put the test in pandas/tests/dtypes/test_common.py since I couldn't find another place where astype_nansafe was tested. Also adds various other tests for astype_nansafe.

Related: #28438

pandas/core/dtypes/cast.py

jreback · 2019-09-18T11:53:44Z

pandas/core/dtypes/cast.py

@@ -696,6 +698,8 @@ def astype_nansafe(arr, dtype, copy=True, skipna=False):
        if is_object_dtype(dtype):
            return tslib.ints_to_pydatetime(arr.view(np.int64))
        elif dtype == np.int64:


this likely needs to be is_integer_dtype(dtype)

I think this might actually need to be np.int64 since above we check for datetime64 (getting some weird test failures after this change). Looking at the numpy documentation it seems the output of view is "unpredictable" when the precision differs between the data types (the shape of the output can even change).

jreback · 2019-09-18T11:55:25Z

pandas/tests/dtypes/test_common.py

@@ -731,3 +732,11 @@ def test__is_dtype_type_sparse():
    result = np.dtype("int32")
    assert com._is_dtype_type(ser, lambda tipo: tipo == result)
    assert com._is_dtype_type(ser.dtype, lambda tipo: tipo == result)
+
+
+def test__nansafe_nat_to_int():


rename to

test_astype_nansafe; can you parameterize this on all ints at the very least. ideally we would have more testing for this (but can be followup)

For the other (non-int64) dtypes we end up hitting the TypeError here (related to the above comment):

pandas/pandas/core/dtypes/cast.py

Line 707 in b81f433

raise TypeError(

Would you recommend breaking these out into a separate test?

pandas/tests/dtypes/test_common.py

jreback · 2019-09-18T11:56:48Z

is there a user visible level that hits this?

TomAugspurger · 2019-09-18T13:09:15Z

This hits both Series.astype, Index.astype, and DataFrame.astype

In [6]: a = [pd.Timestamp('2000'), pd.NaT]

In [7]: pd.Series(a).astype(int)
Out[7]:
0     946684800000000000
1   -9223372036854775808
dtype: int64

In [8]: pd.Index(a).astype(int)
Out[8]: Int64Index([946684800000000000, -9223372036854775808], dtype='int64')

In [10]: pd.DataFrame({"a": a}).astype(int)
Out[10]:
                     a
0   946684800000000000
1 -9223372036854775808

In addition to the astype_nansafe unit tests, we'd ideally also test these user facing ones.

And we should double check: we want this behavior right? Some people may be relying on this behavior (treating pd.NaT as iNaT). This feels like a bigger change than https://github.com/pandas-dev/pandas/pull/28438/files. I might prefer a warning first that this will change (though how would people work around it?).

jreback · 2019-09-20T12:53:06Z

pandas/tests/dtypes/test_common.py

@@ -706,3 +707,13 @@ def test__get_dtype_fails(input_param, expected_error_message):
 )
 def test__is_dtype_type(input_param, result):
    assert com._is_dtype_type(input_param, lambda tipo: tipo == result)
+
+
+@pytest.mark.parametrize("val", [np.datetime64("NaT"), np.timedelta64("NaT")])


can you add an additional test for when dtype == object (which is the other path that hits M8 and m8)

This would be a test separate from test_astype_nansafe to check that NaT is still null after casting to object?

TomAugspurger · 2019-09-20T12:58:04Z

Did we decide on whether we're going to deprecate this behavior before changing it? IMO, ew should

dsaxton · 2019-09-20T14:07:03Z

Did we decide on whether we're going to deprecate this behavior before changing it? IMO, ew should

I'd defer to you guys on that one. If we deprecate, the change would be to use a deprecation warning instead of an error and xfail the tests?

jreback · 2019-09-26T14:28:07Z

Did we decide on whether we're going to deprecate this behavior before changing it? IMO, ew should

@TomAugspurger you feel strongly about this? I don't think its a big deal, but we could deprecate.

TomAugspurger · 2019-09-26T14:30:59Z

Not *too* strongly, just a slight preference. I could go either way.

…

On Thu, Sep 26, 2019 at 9:28 AM Jeff Reback ***@***.***> wrote: Did we decide on whether we're going to deprecate this behavior before changing it? IMO, ew should @TomAugspurger <https://github.com/TomAugspurger> you feel strongly about this? I don't think its a big deal, but we could deprecate. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#28492?email_source=notifications&email_token=AAKAOIT4X7EAX2U27B7IYLDQLTBH7A5CNFSM4IXYP7Z2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7VYO7I#issuecomment-535529341>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKAOIXMNRQ27VC6JNSNIRLQLTBH7ANCNFSM4IXYP7ZQ> .

dsaxton · 2019-09-26T19:04:09Z

Given how unusual the current behavior is, I'd be surprised if anyone was relying on it. I'd say it's more likely to be a source of unexpected bugs.

WillAyd · 2019-10-11T22:29:47Z

Is there a legitimate use case for using astype that we know of? The rules here wouldn't even match numpy right?

Does seem kind of buggy so I think OK without deprecation too

WillAyd · 2019-11-07T23:20:15Z

@dsaxton can you merge master to be sure? I think we can move forward with this as is, unless @TomAugspurger you feel otherwise

jreback · 2019-11-20T13:48:12Z

can you merge master

WillAyd · 2019-12-17T17:51:58Z

@jreback all green - OK to merge?

jreback · 2019-12-27T19:52:05Z

can you merge master and we'll look again

jreback · 2020-01-01T16:23:40Z

lgtm. @jbrockmendel if you'd have a glance (and merge if ok)

jbrockmendel · 2020-01-01T17:02:18Z

thanks @dsaxton

* BUG: Raise when casting NaT to int * Add release note * Add PR number * Use isna * Parametrize test * Check all integer dtypes * Fix merge * Edit release note * Check for np.int64 * Handle timedelta64 * Add astype_nansafe datetime tests * Add test for NaT object casting

Daniel Saxton added 3 commits September 17, 2019 23:04

BUG: Raise when casting NaT to int

0c035bc

Add release note

0d64522

Add PR number

3d91a8f

jreback requested changes Sep 18, 2019

View reviewed changes

pandas/tests/dtypes/test_common.py Outdated Show resolved Hide resolved

jreback added the Dtype Conversions Unexpected or buggy dtype conversions label Sep 18, 2019

Daniel Saxton added 8 commits September 18, 2019 14:06

Use isna

482d629

Parametrize test

975de26

Check all integer dtypes

3445dc7

Merge branch 'master' into cast-nat

2e9ece9

Fix merge

e23b9a0

Edit release note

b14d752

Check for np.int64

9419744

Handle timedelta64

b81f433

dsaxton mentioned this pull request Sep 19, 2019

TST: Add astype_nansafe datetime tests #28533

Closed

jreback requested changes Sep 20, 2019

View reviewed changes

Daniel Saxton added 3 commits September 20, 2019 10:08

Merge branch 'master' into cast-nat

e5dfa71

Add astype_nansafe datetime tests

18734ff

Add test for NaT object casting

c9ba768

jreback added this to the 1.0 milestone Sep 26, 2019

Merge branch 'master' into cast-nat

5da8246

Daniel Saxton added 2 commits November 20, 2019 13:10

Merge branch 'master' into cast-nat

ac501f6

Merge branch 'master' into cast-nat

e229844

Merge branch 'master' into cast-nat

931e8c5

jreback approved these changes Jan 1, 2020

View reviewed changes

jbrockmendel merged commit 27b713b into pandas-dev:master Jan 1, 2020

dsaxton deleted the cast-nat branch January 1, 2020 18:52

TomAugspurger mentioned this pull request Feb 3, 2020

regression: ts64->int64 #31634

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Raise when casting NaT to int #28492

BUG: Raise when casting NaT to int #28492

dsaxton commented Sep 18, 2019 •

edited

Loading

jreback Sep 18, 2019

dsaxton Sep 19, 2019 •

edited

Loading

jreback Sep 18, 2019

dsaxton Sep 19, 2019

jreback commented Sep 18, 2019

TomAugspurger commented Sep 18, 2019 •

edited

Loading

jreback Sep 20, 2019

dsaxton Sep 20, 2019

jreback Sep 25, 2019

TomAugspurger commented Sep 20, 2019

dsaxton commented Sep 20, 2019

jreback commented Sep 26, 2019

TomAugspurger commented Sep 26, 2019 via email

dsaxton commented Sep 26, 2019

WillAyd commented Oct 11, 2019

WillAyd commented Nov 7, 2019

jreback commented Nov 20, 2019

WillAyd commented Dec 17, 2019

jreback commented Dec 27, 2019

jreback commented Jan 1, 2020

jbrockmendel commented Jan 1, 2020

BUG: Raise when casting NaT to int #28492

BUG: Raise when casting NaT to int #28492

Conversation

dsaxton commented Sep 18, 2019 • edited Loading

jreback Sep 18, 2019

Choose a reason for hiding this comment

dsaxton Sep 19, 2019 • edited Loading

Choose a reason for hiding this comment

jreback Sep 18, 2019

Choose a reason for hiding this comment

dsaxton Sep 19, 2019

Choose a reason for hiding this comment

jreback commented Sep 18, 2019

TomAugspurger commented Sep 18, 2019 • edited Loading

jreback Sep 20, 2019

Choose a reason for hiding this comment

dsaxton Sep 20, 2019

Choose a reason for hiding this comment

jreback Sep 25, 2019

Choose a reason for hiding this comment

TomAugspurger commented Sep 20, 2019

dsaxton commented Sep 20, 2019

jreback commented Sep 26, 2019

TomAugspurger commented Sep 26, 2019 via email

dsaxton commented Sep 26, 2019

WillAyd commented Oct 11, 2019

WillAyd commented Nov 7, 2019

jreback commented Nov 20, 2019

WillAyd commented Dec 17, 2019

jreback commented Dec 27, 2019

jreback commented Jan 1, 2020

jbrockmendel commented Jan 1, 2020

dsaxton commented Sep 18, 2019 •

edited

Loading

dsaxton Sep 19, 2019 •

edited

Loading

TomAugspurger commented Sep 18, 2019 •

edited

Loading