Skip to content

Bug fix - Astype Timedelta64[ns] fails when np.nan is included #45965

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

Merged
merged 13 commits into from
Feb 27, 2022

Conversation

weikhor
Copy link
Contributor

@weikhor weikhor commented Feb 12, 2022

@@ -134,7 +134,8 @@ def astype_nansafe(
raise TypeError(f"cannot astype a timedelta from [{arr.dtype}] to [{dtype}]")

elif np.issubdtype(arr.dtype, np.floating) and np.issubdtype(dtype, np.integer):
Copy link
Member

Choose a reason for hiding this comment

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

i think use is_integer_dtype(dtype) instead of issubdtype would be simpler

@@ -482,3 +482,10 @@ def test_getitem_str_second_with_datetimeindex():
msg = r"Timestamp\('2012-01-02 18:01:02-0600', tz='US/Central', freq='S'\)"
with pytest.raises(KeyError, match=msg):
df[df.index[2]]


Copy link
Contributor

Choose a reason for hiding this comment

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

these are astype tests, pls move them.

def test_astype_timedelta64_with_np_nan():
# GH45798
result = Series([1, np.nan]).astype("timedelta64[ns]")
expected = Series([1, pd.NaT]).astype("timedelta64[ns]")
Copy link
Contributor

Choose a reason for hiding this comment

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

to be explictily construct here the expected

Copy link
Contributor Author

@weikhor weikhor Feb 16, 2022

Choose a reason for hiding this comment

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

Hi I do not understand. Can please give more details? Thank.

Copy link
Member

Choose a reason for hiding this comment

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

if the method being tested is Series.astype, then the test belongs in tests.series.methods.test_astype

@weikhor weikhor requested a review from jbrockmendel February 26, 2022 03:36
@jreback jreback added Dtype Conversions Unexpected or buggy dtype conversions Timedelta Timedelta data type labels Feb 27, 2022
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

great, can you add a whatsnew note in 1.5 bug fixes in the timedelta section (or dtype conversion ok too).

pls merge master and ping on green.

@jreback jreback added this to the 1.5 milestone Feb 27, 2022
@weikhor weikhor changed the title Solve Astype Timedelta64[ns] fails when np.nan is included Bug fix - Astype Timedelta64[ns] fails when np.nan is included Feb 27, 2022
@weikhor
Copy link
Contributor Author

weikhor commented Feb 27, 2022

@jreback @jbrockmendel
Hi The test pipeline check in green.

@weikhor weikhor requested a review from jreback February 27, 2022 13:16
@jreback jreback merged commit 429f294 into pandas-dev:main Feb 27, 2022
@jreback
Copy link
Contributor

jreback commented Feb 27, 2022

thanks @weikhor

@@ -284,7 +284,7 @@ Datetimelike

Timedelta
^^^^^^^^^
-
- Bug in :func:`astype_nansafe` astype("timedelta64[ns]") fails when np.nan is included (:issue:`45798`)
Copy link
Member

Choose a reason for hiding this comment

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

is there a user-facing method we can refer to?

yehoshuadimarsky pushed a commit to yehoshuadimarsky/pandas that referenced this pull request Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Dtype Conversions Unexpected or buggy dtype conversions Timedelta Timedelta data type
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: astype("timedelta64[ns]") fails when np.nan is included
3 participants