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

BUG: datetime64 series reduces to nan when empty instead of nat #11245

Merged
merged 1 commit into from Oct 11, 2015

Conversation

Projects
None yet
2 participants
@llllllllll
Contributor

llllllllll commented Oct 5, 2015

I ran into some strange behavior with a series of dtype datetime64[ns] where I called max and got back a nan. I think the correct behavior here is to return nat. I looked through test_nanops but I am not sure where the correct place to put the test for this is.

The new behavior is:

In [1]: pd.Series(dtype='datetime64[ns]').max()
Out[1]: NaT

where the old behavior was:

In [1]: pd.Series(dtype='datetime64[ns]').max()
Out[1]: nan
@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Oct 5, 2015

Contributor

can I add a test in test_nanops and/or test_series

Contributor

jreback commented Oct 5, 2015

can I add a test in test_nanops and/or test_series

@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Oct 5, 2015

Contributor

also want to test for timedelta64/datetime64[ns, tz] too

Contributor

jreback commented Oct 5, 2015

also want to test for timedelta64/datetime64[ns, tz] too

@llllllllll

This comment has been minimized.

Show comment
Hide comment
@llllllllll

llllllllll Oct 5, 2015

Contributor

Sure, I will update this tonight when I get home

Contributor

llllllllll commented Oct 5, 2015

Sure, I will update this tonight when I get home

@jreback jreback added this to the 0.17.1 milestone Oct 5, 2015

@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Oct 5, 2015

Contributor

pls also add a note in the 0.17.1 whatsnew bug section

Contributor

jreback commented Oct 5, 2015

pls also add a note in the 0.17.1 whatsnew bug section

@llllllllll

This comment has been minimized.

Show comment
Hide comment
@llllllllll

llllllllll Oct 7, 2015

Contributor

ping @jreback

Contributor

llllllllll commented Oct 7, 2015

ping @jreback

@@ -637,7 +619,7 @@ def _maybe_null_out(result, axis, mask):
else:
result = result.astype('f8')
result[null_mask] = np.nan
else:
elif result is not tslib.NaT:

This comment has been minimized.

@jreback

jreback Oct 7, 2015

Contributor

if we started with M8/m8 and then do a .view('i8') this needs to be compared to pd.lib.iNaT, not sure why you are not hitting this here

@jreback

jreback Oct 7, 2015

Contributor

if we started with M8/m8 and then do a .view('i8') this needs to be compared to pd.lib.iNaT, not sure why you are not hitting this here

This comment has been minimized.

@llllllllll

llllllllll Oct 7, 2015

Contributor

so this should be elif not (result is tslib.NaT or result is tslib.iNaT)?

@llllllllll

llllllllll Oct 7, 2015

Contributor

so this should be elif not (result is tslib.NaT or result is tslib.iNaT)?

This comment has been minimized.

@jreback

jreback Oct 7, 2015

Contributor

== tslib.iNaT

but puzzled why a NaT is there
as I don't think it's wrapped yet

@jreback

jreback Oct 7, 2015

Contributor

== tslib.iNaT

but puzzled why a NaT is there
as I don't think it's wrapped yet

This comment has been minimized.

@llllllllll

llllllllll Oct 7, 2015

Contributor

If you take a ts and take a view as an int and then reduce, I think you still want nan. The only reason that you would want a NaT is that the dtype of the sequence being reduced is datetime64.

@llllllllll

llllllllll Oct 7, 2015

Contributor

If you take a ts and take a view as an int and then reduce, I think you still want nan. The only reason that you would want a NaT is that the dtype of the sequence being reduced is datetime64.

This comment has been minimized.

@llllllllll

llllllllll Oct 7, 2015

Contributor

Oh, I see what you are saying, you were suggesting:

elif result.view('i8') == tslib.iNaT

It looks like result is still just a timestamp at this point so it will be the NaT object. I don't know if this is a guarantee or not.

@llllllllll

llllllllll Oct 7, 2015

Contributor

Oh, I see what you are saying, you were suggesting:

elif result.view('i8') == tslib.iNaT

It looks like result is still just a timestamp at this point so it will be the NaT object. I don't know if this is a guarantee or not.

This comment has been minimized.

@jreback

jreback Oct 7, 2015

Contributor

no I mean result should already be an int if it's M8 as wrapping is the last step

@jreback

jreback Oct 7, 2015

Contributor

no I mean result should already be an int if it's M8 as wrapping is the last step

This comment has been minimized.

@jreback

jreback Oct 7, 2015

Contributor

ok then

@jreback

jreback Oct 7, 2015

Contributor

ok then

@jreback

View changes

Show outdated Hide outdated pandas/tests/test_dtypes.py Outdated
@jreback

View changes

Show outdated Hide outdated pandas/tests/test_series.py Outdated
@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Oct 7, 2015

Contributor

question for you. otherwise lgtm, pls squash to a single commit (I know different from other projects, just convention here)

Contributor

jreback commented Oct 7, 2015

question for you. otherwise lgtm, pls squash to a single commit (I know different from other projects, just convention here)

@jreback

View changes

Show outdated Hide outdated pandas/core/nanops.py Outdated
result = _wrap_results(result, dtype)
return _maybe_null_out(result, axis, mask)
result = _wrap_results(result, dtype)
return _maybe_null_out(result, axis, mask)

This comment has been minimized.

@llllllllll

llllllllll Oct 7, 2015

Contributor

We have already wrapped the types by the time we call maybe_null_out. The result will already be coerced so I think the is check is safe.

@llllllllll

llllllllll Oct 7, 2015

Contributor

We have already wrapped the types by the time we call maybe_null_out. The result will already be coerced so I think the is check is safe.

This comment has been minimized.

@jreback

jreback Oct 7, 2015

Contributor

ok, then your check is good. thxs

@jreback

jreback Oct 7, 2015

Contributor

ok, then your check is good. thxs

@llllllllll

This comment has been minimized.

Show comment
Hide comment
@llllllllll

llllllllll Oct 7, 2015

Contributor

Also I removed the workaround branch so hopefully the tests pass, otherwise we can put that branch back.

Contributor

llllllllll commented Oct 7, 2015

Also I removed the workaround branch so hopefully the tests pass, otherwise we can put that branch back.

@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Oct 7, 2015

Contributor

ok looks good. I'll be merging things on friday after releasing 0.17.0

Contributor

jreback commented Oct 7, 2015

ok looks good. I'll be merging things on friday after releasing 0.17.0

@llllllllll

This comment has been minimized.

Show comment
Hide comment
@llllllllll

llllllllll Oct 7, 2015

Contributor

Thank you very much

Contributor

llllllllll commented Oct 7, 2015

Thank you very much

@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Oct 7, 2015

Contributor

@llllllllll no thank you!

Contributor

jreback commented Oct 7, 2015

@llllllllll no thank you!

@llllllllll

This comment has been minimized.

Show comment
Hide comment
@llllllllll

llllllllll Oct 10, 2015

Contributor

@jreback just rebased against master, good to merge?

Contributor

llllllllll commented Oct 10, 2015

@jreback just rebased against master, good to merge?

@jreback

View changes

Show outdated Hide outdated doc/source/whatsnew/v0.17.1.txt Outdated
@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Oct 10, 2015

Contributor

we have lots of prs that are worked on at the same time
so when someone adds a note in whatsnew if they are all at the end
merging one requires everyone to rebase
but if the notes are inserted in a big list they are resolved by git
so we can merge many things w/o conflicts

Contributor

jreback commented Oct 10, 2015

we have lots of prs that are worked on at the same time
so when someone adds a note in whatsnew if they are all at the end
merging one requires everyone to rebase
but if the notes are inserted in a big list they are resolved by git
so we can merge many things w/o conflicts

@llllllllll

This comment has been minimized.

Show comment
Hide comment
@llllllllll

llllllllll Oct 10, 2015

Contributor

Ah, that's a cool trick

Contributor

llllllllll commented Oct 10, 2015

Ah, that's a cool trick

@jreback

View changes

Show outdated Hide outdated doc/source/whatsnew/v0.17.1.txt Outdated
@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Oct 11, 2015

Contributor

minor comment, ping when green

Contributor

jreback commented Oct 11, 2015

minor comment, ping when green

BUG: datetime64 series reduces to nan when empty instead of nat
Fixes the parser for datetimetz to also allow the `M8[ns, tz]` alias.
@llllllllll

This comment has been minimized.

Show comment
Hide comment
@llllllllll

llllllllll Oct 11, 2015

Contributor

tests are passing

Contributor

llllllllll commented Oct 11, 2015

tests are passing

jreback added a commit that referenced this pull request Oct 11, 2015

Merge pull request #11245 from llllllllll/dt64-reduce-to-nat
BUG: datetime64 series reduces to nan when empty instead of nat

@jreback jreback merged commit a4843cb into pandas-dev:master Oct 11, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Oct 11, 2015

Contributor

thank you sir!

Contributor

jreback commented Oct 11, 2015

thank you sir!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment