MAINT: use the define for NPY_NO_DEPRECATED_API, fix where needed #10840
Conversation
mattip
changed the title
|
What is the minimal version of NumPy that is supported? |
|
|
it seems i needed to build with |
|
.. and build with cython==0.29.13 not the unreleased head which fixes all the ndarray.ndim -> PyArray_NDIM(ndarray) problems |
If the fix is imminent in Cython for something, waiting for that and bumping the Cython version to >=0.29.14 in this PR would be fine if that's what you'd prefer. |
We just bumped to 0.29.13 in #10943. Might be worth looking at the discussions there to see if 0.29.14 would make sense. |
|
Cython 0.29.14 was released a few days ago... |
|
@mattip Do you mind rebasing this? |
|
I could, but unfortunately Cython 0.29.14 did not include the fixes I was hoping for. That will apparently be part of cython 3 (search for "Properties can be defined for external extension types."). So I could change all the calls to |
Does anyone know when 3.0 is due? I tried a bit of searching and didn't find anything immediately. The closest was a discussion in August about remaining issues: https://mail.python.org/pipermail/cython-devel/2019-August/005258.html The current changelog page page at least lists the release as |
Yes, seems reasonable. |
|
I would suggest to merge at least all the nice API usage changes in order to avoid even more merge conflicts in the future, and then keep using the API instead of direct struct access for code changes. When you enable the API macro then is your choice, but yes, you might want to wait for the final release of Cython 3.0, and maybe also a bit of adoption out there (assuming that this also has a user impact for you, which I can't assess right now). |
| <float32_t *>outdists.data) | ||
| _vq(<float32_t *>np.PyArray_DATA(obs), <float32_t *>np.PyArray_DATA(codes), | ||
| ncodes, nfeat, nobs, <int32_t *>np.PyArray_DATA(outcodes), | ||
| <float32_t *>np.PyArray_DATA(outdists)) |
There was a problem hiding this comment.
@scoder Cython 3.0 would make this kind of change unnecessary, so I would prefer to redo this PR once 3.0 is released.
There was a problem hiding this comment.
@mattip Should we bump the milestone again? I don't think we have Cython 3.x stable release yet? I believe there have been alpha/beta versions available via i.e., PyPI for quite some time now though.
There was a problem hiding this comment.
Yes, let's only continue on this when cpython3.0 is fully released
There was a problem hiding this comment.
As far as I can tell Cython 3.0 has not been released, so bumping the milestone per discussion above.
|
@mattip this would be extremely useful to merge. Cython 3.0 is still too far off, and getting rid of like half of all our build warnings is quite valuable. What do you think about resolving conflicts here and merging in its current state? |
|
If possible, +1 on merging and firefighting afterwards if need be. This is a long-sought-after change that we have been delaying for many major versions. |
|
@scoder thoughts? |
I was hoping to get around a comment when this discussion restarted, but well… ;-) I agree with Matti that this adds a lot of explicit C-API code that you could avoid if you started depending on Cython 3, for which a stable release, as Ralf noted, probably won't happen all too soon. It's your decision whether you want that dependency now already or not. If you want to switch to Cython 3 at some point, then following the development now and depending on exactly the next alpha release (and then future follow-up releases after you tested them) shouldn't induce too much trouble along the way. It's mostly a question of your own project constraints and needs, your distribution scheme, etc. As always, you can generate the C code, test it and ship it. You don't need to tell your users that the code was generated by an alpha version of Cython, and it won't just arbitrarily explode in your hands once it's known to work. If that's not what you want to do, then the question is whether you can wait, or want to make the changes in this PR timely so that you can benefit from less warnings now. These changes won't break once you switch to Cython 3, they'll just add a bit of (shallow) complexity to your code now. You can try to clean that up at some later time, at once or step by step, or you can leave the code as it is then. Both paths seem viable to me, with an (obviously entirely unbiased) preference for the first, since it seems a better investment into the future. And it would allow you to make use of, you know, all those cool new features in Cython 3. :) |
|
+1 for using the latest cython 3.0 alpha release in scipy. Should I refactor the PR to do that? |
While I personally don't have an issue using an alpha, it's pretty problematic to do so in a release; you cannot properly express that dependency in a way that works across install & packaging tools. So I don't think we can do this. I'm not sure what the problem is in just merging this and opening an issue that says "when Cython 3.0 is released, revisit PR 10840 and undo the new unnecessary things there"? |
|
Also if we do this and alpha 9 or beta 1 reveals a tiny issue with the usage then we might have to touch these anyways (small but nonzero probability) . If we are going to touch then better touch when cython 3 is out. Meaning option 2 of @scoder seems better to me |
I stopped doing the busy-work search and replace quite early in the process. If someone else wants to take it over they are welcome. |
Ah okay, I thought this was closer to ready. I'd be interested to rebase it, and even merge the pieces that are ready. Having a clean(er) build log is really helpful when working on build system changes. |
|
Can this be closed now that we require Cython >= 3.0.3? I can't quite tell if this PR was just about build warnings or otherwise worthwhile improvements, but I think we have squashed all Cython build warnings on Edit: closing anyway since @mattip said
|
|
There are still fixes here to Cython code that are relevant, and we need to do build system updates to ensure the old NumPy API cannot be used. @mattip not sure if you have an interest in revisiting this? If not, I can do it. |
|
I have had this on my TODO list since Cython3 came out, and would be happy to hand it off. I think closing this and opening a new PR based on some of these changes would be the right way to proceed. |
|
Okay, I'll do this, thanks Matti. |
This is a follow-up to scipygh-10840.
This is a follow-up to scipygh-10840.
This is a follow-up to scipygh-10840.
This is a follow-up to scipygh-10840.
Build on #4351. Now that the "new" numpy API is quite mature, use it throughout the code. There was only one place I could not use the 1.7 API: in
scipy/interpolate/src/_fitpackmodule.c