-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: isnull doesnt handle PeriodNaT properly #7557
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
Conversation
@@ -237,6 +239,9 @@ cpdef checknull_old(object val): | |||
def isscalar(object val): | |||
return np.isscalar(val) or val is None or PyDateTime_Check(val) | |||
|
|||
cdef is_periodnat(object val): | |||
from tseries.period import Period |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm...this import is a problem, maybe (hacky), check for attributes freq
and ordinal
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can simply cheat hear and do:
return hasattr(val,'ordinal') and val.ordinal == iNaT
and you can make this inline bint as well
see my comments, maybe run a perf check to see if the |
Thanks. I've ran perf after changing your point, and found some string related methods take 2 times slower than latest commit. Maybe every call of |
ok, maybe run with those problematic tests again, they are somewhat random (but the import may be a problem).lmk |
pushing, if you get too it in next day or 2 pls update |
@sinhrks perf on this? |
@sinhrks perf on this change? |
@sinhrks status? |
1 similar comment
@sinhrks status? |
@sinhrks ? |
@sinhrks can you rebase / revisit |
closing pls reopen if/when updated |
Yep, this is mostly solved by #9134. There is still an issue in object dtype, but currently I don't have good idea to handle it properly without affecting to performance (and I think below example is not popular case...).
|
Related to #7485,
isnull
andnotnull
is required to be changed.