TST: fix deprecation warnings from numpy dev's __array_wrap__ signature#15925
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
|
👋 Thank you for your draft pull request! Do you know that you can use |
|
I requested review from @mhvk since he is really the numpy expert here. |
|
Remaining failures are unrelated, and reported as #15926 |
mhvk
left a comment
There was a problem hiding this comment.
Arrggh, I see I didn't actually submit my comments. Now doing so. You'll see one requested change for table, and a question about what to do with convolution, which may need @larrybradley's input.
p.s. Since it keeps the status quot it, I'd be happy to just go with your change for convolution for now if @larrybradley does not have time (or we need to think about it more).
| self.info = obj.info | ||
|
|
||
| def __array_wrap__(self, out_arr, context=None): | ||
| def __array_wrap__(self, out_arr, context=None, return_scalar=False): |
There was a problem hiding this comment.
I think you want to pass on return_scalar in the super() call below.
Also the semantics of the change is that one should do
return self.data[()] if return_scalar else self.data
There was a problem hiding this comment.
fixed ! thank you !
There was a problem hiding this comment.
Ah, good point about the backward compatibility.
Could you also change the if statement on (new) line 749 to look like
if ((NUMPY_LT_2_0 or return_scalar)
and (self.shape != out_arr.shape
or (isinstance(out_arr, BaseColumn)
and (context is not None and context[0] in _comparison_functions)
)
)
):
| self.info = obj.info | ||
|
|
||
| def __array_wrap__(self, obj, context=None): | ||
| def __array_wrap__(self, obj, context=None, return_scalar=False): |
There was a problem hiding this comment.
This one is just right - for Quantity, we never return scalars.
astropy/convolution/core.py
Outdated
| return self._array | ||
|
|
||
| def __array_wrap__(self, array, context=None): | ||
| def __array_wrap__(self, array, context=None, return_scalar=False): |
There was a problem hiding this comment.
This one is a little weird, as I'm not sure which numpy functions are allowed if ufuncs are not - if one simply does not want to look like an array, then I think __array__ = None would do it. Or if it is just ufunc, then __array_ufunc__ = None.
However, I notice that the log gives no warnings about this __array_wrap__, so perhaps it is never used? I guess the coverage would tell... If so, maybe best just to remove it? cc @larrybradley
There was a problem hiding this comment.
Looks like this was added by @adonath many years ago to fix something:
There was a problem hiding this comment.
It looks like __array_wrap__ was added to allow a Kernel object (stored internally as an array) to be multiplied by a scalar. Is there a better way to do that?
There was a problem hiding this comment.
Ah, yes, that makes sense. Two solutions: (1) set __array_priority__ = 1000, so that you get precedence, (2) set __array_ufunc__ = None to indicate numpy should not try to do any arithmetic. The latter is the more "modern" one and probably best here - one can always revisit if it is decided to allow more than the basic operations. But obviously check whether the tests still pass!
There was a problem hiding this comment.
let's try __array_ufunc__ = None
681436a to
013f551
Compare
013f551 to
595cc4d
Compare
595cc4d to
b84aa27
Compare
b84aa27 to
5025468
Compare
|
I think this one is go ? |
|
To add to the fun, now https://datacenter.iers.org/data/9/finals2000A.all is down... Hopefully temporary. But unrelated to numpy-dev. |
This comment was marked as resolved.
This comment was marked as resolved.
|
Anyone able to manually backport? Thanks! 🙏 |
|
On it. |
…wrap_return_scalar TST: fix deprecation warnings from numpy dev's __array_wrap__ signature (cherry picked from commit 316f8b9)
Backport PR #15925 on branch v6.0.x (TST: fix deprecation warnings from numpy dev's __array_wrap__ signature)
Description
Fixes #15924
The discussion upstream is pretty rich so I haven't read it all, but the docstring says
I'm just going with the first option for now, but I don't think I understand the problem solved by this new argument deeply enough to make a call between the two proposed approaches. Since @mhvk served as a reviewer to numpy/numpy#25409, I assume he's in a better position to make the call.