Skip to content

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

Closed
wants to merge 1 commit into from

Conversation

sinhrks
Copy link
Member

@sinhrks sinhrks commented Jun 24, 2014

Related to #7485, isnull and notnull is required to be changed.

@@ -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
Copy link
Contributor

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?

Copy link
Contributor

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

@jreback
Copy link
Contributor

jreback commented Jun 24, 2014

see my comments, maybe run a perf check to see if the is_periodnat is an issue

@jreback jreback added Bug and removed Bug labels Jun 24, 2014
@jreback jreback added this to the 0.14.1 milestone Jun 24, 2014
@sinhrks
Copy link
Member Author

sinhrks commented Jun 26, 2014

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 is_periodnat from checknullobj is problematic, I'll consider better approach.

@jreback
Copy link
Contributor

jreback commented Jun 26, 2014

ok, maybe run with those problematic tests again, they are somewhat random (but the import may be a problem).lmk

@jreback
Copy link
Contributor

jreback commented Jul 6, 2014

pushing, if you get too it in next day or 2 pls update

@jreback jreback modified the milestones: 0.15.0, 0.14.1, 0.15.1 Jul 6, 2014
@jreback
Copy link
Contributor

jreback commented Jul 21, 2014

@sinhrks perf on this?

@jreback
Copy link
Contributor

jreback commented Sep 8, 2014

@sinhrks perf on this change?

@jreback
Copy link
Contributor

jreback commented Sep 14, 2014

@sinhrks status?

1 similar comment
@jreback
Copy link
Contributor

jreback commented Sep 26, 2014

@sinhrks status?

@jreback
Copy link
Contributor

jreback commented Sep 29, 2014

@sinhrks ?

@jreback jreback modified the milestones: 0.15.1, 0.15.0 Sep 29, 2014
@jreback
Copy link
Contributor

jreback commented Jan 18, 2015

@sinhrks can you rebase / revisit

@jreback jreback modified the milestones: 0.16.0, Next Major Release Mar 3, 2015
@jreback
Copy link
Contributor

jreback commented May 9, 2015

closing pls reopen if/when updated

@jreback jreback closed this May 9, 2015
@sinhrks
Copy link
Member Author

sinhrks commented May 9, 2015

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...).

idx = pd.Index(['x', pd.Period('NaT', freq='M')])
pd.isnull(idx)
# [False False]

@sinhrks sinhrks deleted the pnat_isnull branch November 14, 2015 05:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate Period Period data type
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants