Avoid getting DeprecationWarnings on broadcasted arrays fed into Cython#8965
Avoid getting DeprecationWarnings on broadcasted arrays fed into Cython#8965bsipocz merged 1 commit intoastropy:masterfrom
Conversation
|
I don't think a changelog is necessary, users will have no way to have a say here, this is not different from any of the other compatibility changes we do for new upstream releases. |
bsipocz
left a comment
There was a problem hiding this comment.
This does the job perfectly and also very nice to have the references in comments when we can remove the workaround.
|
Let's hold off on merging a little - there's quite a bit of discussion on how to make things better on the numpy side at numpy/numpy#13929 - it may be that after that, this PR can be simplified further. |
|
I don't think this can be backported cleanly to 2.x due to moving the code to @mhvk , looks like there was a long discussion over at Numpy indeed. Is there a conclusion yet? Thank you very much for looking into this! |
|
This should be an easy backport, git is usually good with recognizing the moves (even if it doesn't the changes are trivial to do manually for a conflicting backport, and to one file only, so I think it's fine as is) |
@mhvk , are we still waiting for some decision upstream at Numpy? |
|
Numpy upstream has merge the fix, but it's still broken with us, so I got confused and lost track whether we need to change things or not: numpy/numpy#13993 (note that this PR is might not been in numpy master yet, but I've tested 1.17.x locally with astropy master, and nothing has changed) |
|
I think we still need the |
|
ah, ok, indeed, that's now clearer. |
bb60083 to
2c78efa
Compare
|
I pushed up the more minimal change. I checked that it works locally on 1.17rc2 (but numpy-dev is not up to date yet). I think we can now just merge this... |
|
Actually, let me remove the gratuitous extra line in core.py |
…on code. Declare them const so no check on writing is done.
2c78efa to
53643b8
Compare
|
OK, now it's just lombscargle (which was under |
|
though not sure we care about numpy 1.17 for astropy 2.0... |
so far we're OK, so will backport this to LTS, too. |
pllim
left a comment
There was a problem hiding this comment.
LGTM. I think this is ok to merge if numpy dev job passes. 🚀
it won't as they haven't yet forward ported the fix to master yet. |
|
Yes, my sense would be just to merge as is if tests pass, since I tested 1.17rc2 locally. |
|
Thanks for the fix @mhvk, this was a very nice learning experience between the projects (though any understanding of numpy C is way above my level). |
|
Indeed, goes to show it is worth reporting issues upstream! Certainly, far from an obvious fix to me too... |
Avoid getting DeprecationWarnings on broadcasted arrays fed into Cython
Avoid getting DeprecationWarnings on broadcasted arrays fed into Cython
Hopefully will fix (most of?) #8935. Note that this goes the route of explicitly declaring broadcast arrays as
readonly, which should be futureproof and guarantees a bit more that we're not messing things up inadvertently.Note: check
numpy-devrun in particular!p.s. Can add a change-log entry if people feel it is useful - noting that users will generally not see the
DeprecationWarning