Skip to content
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

Remove out-of-date numpy.pxd; remove unused float16_t #18101

Closed
wants to merge 3 commits into from

Conversation

jbrockmendel
Copy link
Member

  • closes #xxxx
  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

@jbrockmendel
Copy link
Member Author

Just updated the imports of NPY_DATETIME and NPY_TIMEDELTA. Noticed there is some redundancy in the places they are used:

tslib:

cpdef object get_value_box(ndarray arr, object loc):
    [...16 lines suppressed...]
    if arr.descr.type_num == NPY_DATETIME:
        return Timestamp(util.get_value_1d(arr, i))
    elif arr.descr.type_num == NPY_TIMEDELTA:
        return Timedelta(util.get_value_1d(arr, i))
    else:
        return util.get_value_1d(arr, i)

index:

def get_value_at(ndarray arr, object loc):
    if arr.descr.type_num == NPY_DATETIME:
        return Timestamp(util.get_value_at(arr, loc))
    elif arr.descr.type_num == NPY_TIMEDELTA:
        return Timedelta(util.get_value_at(arr, loc))
    return util.get_value_at(arr, loc)

[...]
cdef class IndexEngine:
     [...]
    cpdef get_value(self, ndarray arr, object key, object tz=None):
        """
        arr : 1-dimensional ndarray
        """
        cdef:
            object loc
            void* data_ptr

        loc = self.get_loc(key)
        if PySlice_Check(loc) or cnp.PyArray_Check(loc):
            return arr[loc]
        else:
            if arr.descr.type_num == NPY_DATETIME:
                return Timestamp(util.get_value_at(arr, loc), tz=tz)
            elif arr.descr.type_num == NPY_TIMEDELTA:
                return Timedelta(util.get_value_at(arr, loc))
            return util.get_value_at(arr, loc)

Incidentally, index.get_value_at is never used.

@jreback
Copy link
Contributor

jreback commented Nov 4, 2017

pls leave float16_t around, it is a valid type (though only partially supported generally).

@jbrockmendel
Copy link
Member Author

pls leave float16_t around, it is a valid type (though only partially supported generally).

Can't. This PR removes src/numpy.pxd so as to defer to the version found in setup using numpy_incl = pkg_resources.resource_filename('numpy', 'core/include') (equiv np.get_include()). The idea being to avoid conflicting/redundant includes/namespaces.

If I add float16_t back to one of the cimports here, we get a build-time error:

Error compiling Cython file:
------------------------------------------------------------
...
# cython: profile=False

from numpy cimport (ndarray, float16_t, float64_t, int32_t, int64_t, uint8_t, uint64_t)
^
------------------------------------------------------------

pandas/_libs/index.pyx:3:0: 'numpy/float16_t.pxd' not found

Besides which, float16_t is never actually used in any of the modules it is currently cimported into.

... and looking at numpy.pxd, it looks very much like float16_t is... well I don't know what the term is:

cdef extern from "numpy/arrayobject.h":
    [...]
    ctypedef float        npy_float16
    ctypedef float        npy_float32
[...]

ctypedef npy_float16    float16_t

Is this actually an alias for float32_t?

@gfyoung gfyoung added Clean Internals Related to non-user accessible pandas implementation labels Nov 5, 2017
@gfyoung
Copy link
Member

gfyoung commented Nov 5, 2017

Is this actually an alias for float32_t?

Following that logic that you presented, then yes, it would be.

@jreback
Copy link
Contributor

jreback commented Nov 5, 2017

rebase

@jbrockmendel
Copy link
Member Author

I have no idea what's going on with the test failures. Can anyone reproduce locally? I'd also be ok with a "this cleanup can be future-us's problem"

@jbrockmendel
Copy link
Member Author

May be related to #18064.

@jbrockmendel
Copy link
Member Author

No idea what's up with these failures. Closing to focus attention on #17746, #18016, #18034.

@jbrockmendel jbrockmendel deleted the np_dep branch December 8, 2017 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean Internals Related to non-user accessible pandas implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants