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

Partial remove usages of deprecated Numpy API #2559

Closed
wants to merge 2 commits into from

Conversation

jbrockmendel
Copy link
Contributor

@MattiF ref #2498

The remaining problem is that the ndarray class that numpy exposes doesn't have most of the attributes that it used to. So for example I got rid of descr and replaced the one usage of self.descr with PyArray_DESCR(self). But there are other attributes that aren't as easy to replace.

When I install this branch and try to build pandas with it, I get errors of the form:

pandas/_libs/algos.c:19128:36: error: no member named 'dimensions' in 'struct tagPyArrayObject'
  __pyx_v_length = (__pyx_v_index->dimensions[0]);

pandas/_libs/algos.c:71747:34: error: no member named 'strides' in 'struct tagPyArrayObject'
  __pyx_t_9 = (((__pyx_v_values->strides[1]) == (__pyx_v_out.strides[1])) != 0);

Frankly I think the fewer-attributes version is a pain, since it means in downstream code I won't be able to use e.g. arr.shape, but se la vie.

Could we fake the shape attribute as e.g. a property defined in the pxd file?

@robertwb
Copy link
Contributor

I'd rather not special case this, but it may be preferable to fixing all the code out there using these attributes (that, I suppose, use the deprecated API). The switch from attribute access to a macro (?) to access these is less natural in Python than in C.

@mattip
Copy link
Contributor

mattip commented Aug 17, 2018

I think putting the deprecated fields into a

#if !defined(NPY_NO_DEPRECATED_API) || \
    (NPY_NO_DEPRECATED_API < NPY_1_7_API_VERSION)
.... direct declarations here
#endif

would cause direct field access to fail only for people who define NPY_NO_DEPRECATED_API >= NPY_1_7_API_VERSION. Not defining that constant will continue to print out warnings. Hopefully that will annoy them enough to define it to the latest version and fix their code.

Then we could try cython tests with CFLAGS=-DNPY_NO_DEPRECATED_API=NPY_1_14_API_VERSION and without in order to see the warnings dissapear.

At least that is the theory, if I understand it correctly.

@robertwb The whole goal is to move "all the code out there" to using the newer API.

@robertwb
Copy link
Contributor

robertwb commented Aug 17, 2018 via email

@scoder
Copy link
Contributor

scoder commented Aug 18, 2018

Special case these attributes in the numpy.ndarray class

We could also just try to set the NumPy version macro based on that. When we detect that no deprecated attributes are used in the code, we set the macro and the warnings would go away.

@robertwb
Copy link
Contributor

robertwb commented Aug 18, 2018 via email

@jbrockmendel
Copy link
Contributor Author

I'd like to make a pitch for keeping ndim and shape etc attributes available. Besides the annoyance-downstream issue, discussion in #2498 suggests numpy folks may be amenable to bringing back the attributes that were deprecated so that authors can continue having their code Just Work.

(or if we should just push MemoryViews more, if
performance and everything else has been brought up to parity)

Side Note FWIW: I've been transitioning a bunch of pandas code to use memoryviews recently. The main things slowing that transition are:

  • for some functions we need to use np.ndarray.searchsorted (which is a python call, but se la vie)
  • I'm not aware of a memoryview-only equivalent of np.ones(n, dtype=np.int64) (i.e. I know I can do cdef int64_t[:] arr = np.ones(n, dtype=np.int64) but I'd like to do this without a call into python-land)
  • PR reviewers are very picky about anything that impacts perf, and it isn't obvious whether the relevant uses cases see improvements and by how much.

scoder added a commit that referenced this pull request Aug 21, 2018
@scoder
Copy link
Contributor

scoder commented Aug 21, 2018

I applied most of this PR manually in 222334c to avoid backwards incompatible breakage. Specifically, I did not remove the existing declarations, just removed their usage and added the new declarations.

@jbrockmendel
Copy link
Contributor Author

I have no clear way forward on this. Is it worth keeping this open for reference, or close?

@scoder
Copy link
Contributor

scoder commented Sep 1, 2018

Closing this PR since we have #2498 to track the general issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants