Replace (private) _checknull with (public) is_nan #22146
Conversation
Codecov Report
@@ Coverage Diff @@
## master #22146 +/- ##
=======================================
Coverage 92.06% 92.06%
=======================================
Files 169 169
Lines 50694 50694
=======================================
Hits 46671 46671
Misses 4023 4023
Continue to review full report at Codecov.
|
pandas/_libs/tslibs/util.pxd
Outdated
| ------- | ||
| is_nan : bool | ||
| """ | ||
| return isinstance(val, float) and val != val |
There was a problem hiding this comment.
should this be is_float_object (e.g. this excludes compex)
There was a problem hiding this comment.
This is functionally equivalent to the current implementation.
(The current version uses PyFloatCheck(val) instead of isinstance(val, float), but cython converts the latter to the former)
There was a problem hiding this comment.
does this handle complex as well? I would rather use the same check as we do elsewhere, as this is confusing when to use one vs the other.
There was a problem hiding this comment.
does this handle complex as well?
No. This also only checks for python float, as opposed to is_float_object which also checks for numpy float (the examples I've checked with isinstance(np.float64(2.3), float) have come back True, not sure what the Falsey case is).
_libs.missing.checknull begins with the check you might have in mind:
if is_float_object(val) or is_complex_object(val):
return val != val
[...]
ATM _checknull is used in exactly 19 places, at least one of which is in a float-only branch. Based on a quick look it isn't obvious which usages (at least one) are specifically-for-float. I'll take a look.
There was a problem hiding this comment.
After looking these over, I'm inclined to think it makes sense to use is_float_object but continue excluding complex. At least two of the usages are floating-specific, and complex is sufficiently corner-case-y that I'd rather check for it explicitly-or-not-at-all.
It also looks like checknull_with_nat and is_null_period may be unnecessary, will look at that in an upcoming pass.
jreback
added
Missing-data
|
can you rebase and merge on green (just to be sure). |
* make public is_nan instead of private _checknull * cleanup non-py syntax * fixup missed usage * Cleanup import and whitespace * have is_nan check for np.float_ * fix segfaults
util._checknull(x)is equivalent tox is None or isnan(x). On several occasions I have had to go remind myself what_checknullcovers. It is much clearer to write outx is None or util.is_nan(x), which is what this PR does.In at least one place where
_checknullis currently used, theis Nonepart of it can be skipped.Also gets rid of usages of private functions, and trades a no-docstring func for a nice-docstring func.
Small assorted cleanups along the way.