implement libmissing; untangles _libs dependencies #18357

jbrockmendel · 2017-11-18T16:36:34Z

In the status quo, algos, hashtable, parsers, and period depend on lib. On top of that groupby, join, and index depend on algos and intervaltree depends on hashtable. (In fairness, some of these dependency relationships are python imports, not cimports.)

lib depends on tslib which depends on essentially all of tslibs. So we've got a whole bunch of dependencies (many of which aren't declared in setup.py).

But the lib functions needed by algos, hashtable, and period are basically all a small handful of null-checking checnull, isnaobj, isnaobj2d which depend only on util and NaT.

So this PR takes those functions and puts them into _libs.missing, then updates the relevant imports.

codecov · 2017-11-18T20:10:32Z

Codecov Report

Merging #18357 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #18357      +/-   ##
==========================================
- Coverage   91.38%   91.36%   -0.02%     
==========================================
  Files         164      164              
  Lines       49790    49791       +1     
==========================================
- Hits        45501    45493       -8     
- Misses       4289     4298       +9

Flag	Coverage Δ
#multiple	`89.17% <100%> (ø)`	⬆️
#single	`39.5% <50%> (-0.06%)`	⬇️

Impacted Files	Coverage Δ
pandas/io/formats/format.py	`96.01% <100%> (ø)`	⬆️
pandas/core/dtypes/missing.py	`90.62% <100%> (ø)`	⬆️
pandas/io/formats/excel.py	`96.72% <100%> (+0.01%)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.8% <0%> (-0.1%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfad581...5fea502. Read the comment docs.

codecov · 2017-11-18T20:10:33Z

Codecov Report

Merging #18357 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #18357      +/-   ##
==========================================
- Coverage   91.35%   91.33%   -0.02%     
==========================================
  Files         163      163              
  Lines       49714    49716       +2     
==========================================
- Hits        45415    45408       -7     
- Misses       4299     4308       +9

Flag	Coverage Δ
#multiple	`89.13% <100%> (ø)`	⬆️
#single	`39.64% <58.82%> (-0.06%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/dtypes/missing.py	`90.74% <100%> (+0.11%)`	⬆️
pandas/io/formats/excel.py	`96.71% <100%> (ø)`	⬆️
pandas/io/formats/format.py	`96.01% <100%> (ø)`	⬆️
pandas/io/gbq.py	`25% <0%> (-58.34%)`	⬇️
pandas/core/frame.py	`97.8% <0%> (-0.1%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d421a09...e94e01f. Read the comment docs.

jreback · 2017-11-19T15:11:08Z

pandas/_libs/missing.pyx

+        return get_timedelta64_value(val) == NPY_NAT
+    elif util.is_array(val):
+        return False
+    else:


is there a reason for not pulling in util._checknull here as well, as seems logical? (or just future PR)

You mean defining it here instead of in util? Or importing it into the namespace? I'd be +1 on the former, indifferent to the latter.

I think for sure should define it here. but then this puts missing as a dep of things like hashing.pyx. ok with it being a dep of any of the tslibs though.

(pandas) bash-3.2$ find pandas -name '*.pyx' | xargs grep _checknull pandas/_libs/hashing.pyx:from util cimport _checknull pandas/_libs/hashing.pyx: elif _checknull(val): pandas/_libs/interval.pyx: if util._checknull(interval): pandas/_libs/lib.pyx:from util cimport is_array, _checknull, _checknan pandas/_libs/lib.pyx: return _checknull(val) pandas/_libs/lib.pyx: return _checknull(val) pandas/_libs/lib.pyx: result[i] = val is NaT or util._checknull_old(val) pandas/_libs/lib.pyx: _checknull(x) and _checknull(y)): pandas/_libs/lib.pyx: if _checknull(val): pandas/_libs/lib.pyx: if _checknull(x): pandas/_libs/lib.pyx: if _checknull(x): pandas/_libs/lib.pyx: elif _checknull(y): pandas/_libs/src/inference.pyx: if util._checknull(val): pandas/_libs/src/inference.pyx: elif util._checknull(v): pandas/_libs/src/inference.pyx: if util._checknull(v): pandas/_libs/src/inference.pyx: if util._checknull(v): pandas/_libs/src/inference.pyx: if util._checknull(v): pandas/_libs/src/inference.pyx: if util._checknull(v): pandas/_libs/src/inference.pyx: return util._checknull(value) pandas/_libs/src/inference.pyx: bint is_generic_null = util._checknull(value) pandas/_libs/tslib.pyx:from tslibs.nattype cimport _checknull_with_nat, NPY_NAT pandas/_libs/tslib.pyx: if _checknull_with_nat(val): pandas/_libs/tslib.pyx: if _checknull_with_nat(val): pandas/_libs/tslib.pyx: if _checknull_with_nat(val): pandas/_libs/tslib.pyx: if _checknull_with_nat(val): pandas/_libs/tslib.pyx: if _checknull_with_nat(val): pandas/_libs/tslib.pyx: if _checknull_with_nat(val): pandas/_libs/tslibs/nattype.pyx:cdef inline bint _checknull_with_nat(object val): pandas/_libs/tslibs/strptime.pyx:from nattype cimport _checknull_with_nat, NPY_NAT pandas/_libs/tslibs/strptime.pyx: if _checknull_with_nat(val): pandas/_libs/tslibs/timedeltas.pyx:from nattype cimport _checknull_with_nat, NPY_NAT pandas/_libs/tslibs/timedeltas.pyx: if _checknull_with_nat(ts): pandas/_libs/tslibs/timedeltas.pyx: if _checknull_with_nat(other): pandas/_libs/tslibs/timedeltas.pyx: elif _checknull_with_nat(value):

If we're moving util._checknull anyway, I'd advocate renaming it to e.g. check_none_or_nan

I think for sure should define it here. but then this puts missing as a dep of things like hashing.pyx. ok with it being a dep of any of the tslibs though.

I'll take a look and see which util funcs can be moved without messing with dependencies.

FWIW this PR already adds missing to the 'pxdfiles` key of hashtable, which cimports missing.checknull. Previously it was an un-declared dependency on lib.

Looks like util._checknull_old can be moved to missing (is used there once, nowhere else). Let's saving util._checknull for later, since it is used in a bunch of places.

ok with leaving these for later as well

Sounds good. Just pushed commit with docstrings.

jreback · 2017-11-19T15:11:21Z

pandas/_libs/missing.pyx

+@cython.wraparound(False)
+@cython.boundscheck(False)
+def isnaobj(ndarray arr):
+    cdef:


ideally add some doc-strings

jreback · 2017-11-19T15:12:04Z

pandas/_libs/missing.pyx

+cdef int64_t NPY_NAT = util.get_nat()
+
+
+cdef inline bint is_null_datetimelike(v):


prob should rename this for consistency (checknull_datetimelike), can be TODO

jreback · 2017-11-19T15:13:06Z

pandas/_libs/src/inference.pyx

@@ -533,22 +533,6 @@ cpdef object infer_datetimelike_array(object arr):
    return 'mixed'


-cdef inline bint is_null_datetimelike(v):


is there a reason these are not in nattype? (and maybe namespaces to missing, but actual definition in nattype)

I'd be fine putting this in nattype. The one below I considered moving but decided against because it's only used once here in inference.pyx.

jreback · 2017-11-19T15:13:59Z

pandas/_libs/tslib.pyx

@@ -830,24 +829,6 @@ class Timestamp(_Timestamp):
 # ----------------------------------------------------------------------


-cdef inline bint _check_all_nulls(object val):
-    """ utility to check if a value is any type of null """


is this used anywhere?

Yes, its used once here in _libs.missing.

jreback · 2017-11-19T15:14:35Z

pandas/tests/test_lib.py

@@ -208,14 +208,14 @@ class TestNAObj(object):

    def _check_behavior(self, arr, expected):


should move these tests to test_missing

Sure. Existing test_missing in tests.dtypes or a new test_libmissing?

i think tests.dtypes is ok, maybe add test_libmissing at later point (orthogonal to this)

jreback · 2017-11-19T19:59:02Z

pandas/_libs/algos_rank_helper.pxi.in

@@ -83,7 +83,7 @@ def rank_1d_{{dtype}}(object in_arr, ties_method='average', ascending=True,
        nan_value = {{neg_nan_value}}

    {{if dtype == 'object'}}
-    mask = lib.isnaobj(values)
+    mask = missing.isnaobj(values)


these could be cimports instead?

ATM these are not cdef

jreback · 2017-11-19T20:01:44Z

pandas/io/formats/format.py

@@ -1860,7 +1860,7 @@ def _format_strings(self):
            (lambda x: pprint_thing(x, escape_chars=('\t', '\r', '\n'))))

        def _format(x):
-            if self.na_rep is not None and lib.checknull(x):
+            if self.na_rep is not None and libmissing.checknull(x):


in followup (or here), change this to

is_scalar(x) and isna(x); this is reaching too much into the internals.

and all below here

maybe worth defining a is_scalar_na function

jreback · 2017-11-19T20:03:07Z

pandas/core/dtypes/missing.py

@@ -50,7 +50,7 @@ def isna(obj):

 def _isna_new(obj):
    if is_scalar(obj):
-        return lib.checknull(obj)


see this here is ok, as this is really the python api for missing values.

jreback · 2017-11-19T20:03:25Z

pandas/io/formats/excel.py

@@ -381,12 +381,12 @@ def __init__(self, df, na_rep='', float_format=None, cols=None,
        self.inf_rep = inf_rep

    def _format_value(self, val):
-        if lib.checknull(val):
+        if libmissing.checknull(val):


use is_scalar and isna

jreback · 2017-11-19T20:03:54Z

pandas/io/formats/excel.py

            val = self.na_rep
        elif is_float(val):
-            if lib.isposinf_scalar(val):
+            if libmissing.isposinf_scalar(val):


isposinf_scalar should be exposed in core/dtypes/missing.py as a python function

jreback · 2017-11-19T20:04:25Z

pandas/tests/dtypes/test_missing.py

+    _1d_methods = ['isnaobj', 'isnaobj_old']
+    _2d_methods = ['isnaobj2d', 'isnaobj2d_old']
+
+    def _check_behavior(self, arr, expected):


if you can parametrize/use fixtures would be good

…bmissing

jreback · 2017-11-20T11:23:14Z

rebase

…bmissing

jreback · 2017-11-22T01:56:42Z

pls rebase

…bmissing

jreback · 2017-11-22T11:21:56Z

pandas/_libs/missing.pyx

+cimport util
+
+from tslibs.np_datetime cimport get_timedelta64_value, get_datetime64_value
+from tslibs.nattype import NaT, iNaT


instead of importing iNaT, just use NPY_NAT to avoid perf issues.

jreback · 2017-11-22T11:22:06Z

pandas/_libs/missing.pyx

+cdef int64_t NPY_NAT = util.get_nat()
+
+
+cdef inline bint is_null_datetimelike(v):


type as object

…bmissing

jbrockmendel · 2017-11-22T15:42:49Z

instead of importing iNaT, just use NPY_NAT to avoid perf issues.

type as object

Just pushed with both these changes. Also:

changed variable v --> val (there's a recent issue regarding linting too-short variable names that I very much agree with)
changed for i from 0 <= i < n: --> for i in range(n): in a couple places

jreback · 2017-11-22T16:27:17Z

setup.py

@@ -516,6 +517,10 @@ def pxd(name):
    '_libs.lib': {
        'pyxfile': '_libs/lib',
        'depends': lib_depends + tseries_depends},


need to add missing to lib as well

Can do. At the moment we've basically only implemented the pxfiles "rule" for tslibs. Should we go through and fill them out for the others?

In case I haven't mentioned it recently: I'm really looking forward to using cythonize and not worrying about this.

jreback · 2017-11-22T22:52:32Z

thanks!

jreback · 2017-11-22T23:05:52Z

not sure this started showing up on this PR, but pls add to the list

/Users/jreback/miniconda3/envs/pandas/lib/python3.6/site-packages/numpy/core/include/numpy/npy_1_7_deprecated_api.h:15:2: warning: "Using deprecated NumPy API, disable it by "          "#defining NPY_NO_DEPRECATED_API NPY_1_7_API_VERSION" [-W#warnings]
#warning "Using deprecated NumPy API, disable it by " \
 ^
pandas/_libs/src/datetime/np_datetime.c:571:12: warning: expression result unused [-Wunused-value]
  meta.num - 1;
  ~~~~~~~~ ^ ~
pandas/_libs/src/datetime/np_datetime.c:1004:15: warning: unused variable 'perday' [-Wunused-variable]
    npy_int64 perday;
              ^
3 warnings generated.

jbrockmendel · 2017-11-23T01:15:12Z

not sure this started showing up on this PR, but pls add to the list

Looks like these are in the new timedelta_struct functions, likely leftovers from copy/paste. Easy to fix in-place, but I'd advocate not having them in src/ to begin with. Put them directly in tslibs.np_datetime (or tslibs.np_timedelta).

jbrockmendel added 4 commits November 17, 2017 20:29

implement _libs.missing; remove all cimports of libs._lib

12a880e

cimport checknull instead of import

528cec1

fixup import

d6355fe

flake8 fixup

5fea502

jreback requested changes Nov 19, 2017

View reviewed changes

jreback added Clean Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate labels Nov 19, 2017

jbrockmendel mentioned this pull request Nov 19, 2017

tslibs TODO list #17652

Closed

59 tasks

jbrockmendel added 3 commits November 19, 2017 09:23

Move libmissing tests to test_missing

876f848

move util._checknull_old --> missing._check_none_nan_inf_neginf

9dcf97c

add docstrings

2cf1b21

jreback requested changes Nov 19, 2017

View reviewed changes

jbrockmendel added 3 commits November 19, 2017 14:16

edits per reviewer request

44fbe90

Merge branch 'master' of https://github.com/pandas-dev/pandas into li…

f833419

…bmissing

fixup missing import

d2184f1

Merge branch 'master' of https://github.com/pandas-dev/pandas into li…

5614df8

…bmissing

jbrockmendel added 2 commits November 21, 2017 18:08

Merge branch 'master' of https://github.com/pandas-dev/pandas into li…

a688996

…bmissing

remove imports no longer used after merge

d1323fb

jreback requested changes Nov 22, 2017

View reviewed changes

jreback added this to the 0.22.0 milestone Nov 22, 2017

jbrockmendel added 2 commits November 22, 2017 07:36

Merge branch 'master' of https://github.com/pandas-dev/pandas into li…

ce185e0

…bmissing

use NPY_NAT instead of iNaT, range instead of C syntax

6d8308f

jreback approved these changes Nov 22, 2017

View reviewed changes

jreback reviewed Nov 22, 2017

View reviewed changes

add pxdfiles for _libs.lib

e94e01f

jreback merged commit bd145c8 into pandas-dev:master Nov 22, 2017

jbrockmendel deleted the libmissing branch November 23, 2017 01:13

		cdef int64_t NPY_NAT = util.get_nat()


		cdef inline bint is_null_datetimelike(v):

		@@ -533,22 +533,6 @@ cpdef object infer_datetimelike_array(object arr):
		return 'mixed'


		cdef inline bint is_null_datetimelike(v):

		@@ -208,14 +208,14 @@ class TestNAObj(object):

		def _check_behavior(self, arr, expected):

implement libmissing; untangles _libs dependencies #18357

implement libmissing; untangles _libs dependencies #18357

Conversation

jbrockmendel commented Nov 18, 2017

codecov bot commented Nov 18, 2017

Codecov Report

codecov bot commented Nov 18, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel Nov 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Nov 20, 2017

jreback commented Nov 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Nov 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Nov 22, 2017

jreback commented Nov 22, 2017

jbrockmendel commented Nov 23, 2017

codecov bot commented Nov 18, 2017 •

edited

Loading

jbrockmendel Nov 19, 2017 •

edited

Loading