RFC: make all MaskedNDArray.__wrap_array__'s helper functions return a (result, mask, out) tuple (fix compat with numpy dev)#15948
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 |
f8454a2 to
6cc458b
Compare
|
Would be nice if we can turn that failing astroquery case into a test here but without all that server calls, if possible. If not, then maybe try this branch out over at astroquery as a draft PR to be sure: https://github.com/astropy/astroquery/blob/110cb833459b97a6c233bba871447bbeb814b25d/tox.ini#L31 Thanks! |
|
This looks promising! I'll have a better look later... |
I'm not convinced it's worth it: we're already catching the issue seen in astroquery with our current test suite.
I'll do that now ! update: here it is: astropy/astroquery#2931 |
mhvk
left a comment
There was a problem hiding this comment.
I think this is the right approach, but since it affects Masked and not quantity, I'd prefer to just fix it there. Looking at the code, easiest would seem to be to insist dispatched_function always returns a tuple of 3, so if the result is already OK, one should just use result, None, None.
The one exception perhaps would be that returning a plain None is OK (for the various put functions).
What do you think? If OK, that should be a quick update, but don't forget to update the docstring for DISPATCHED_FUNCTIONS as well!
astropy/utils/masked/core.py
Outdated
| return self._not_implemented_or_raise(function, types) | ||
|
|
||
| if not isinstance(dispatched_result, tuple): | ||
| if dispatched_function._direct_results: |
There was a problem hiding this comment.
With my main suggestion, this branch is not necessary any more, but I think I still like the ability to just return None, so this would be
if dispatched_result is None:
return
There was a problem hiding this comment.
p.s. Note that with result, None, None, the fall-through does the right thing, of just returning result. I don't quite understand why it wasn't a tuple of 3 to start with, but likely I just started with returning the plain result, and then realized there were many cases where I had to attach the mask or deal with out, so might as well have the option to return a tuple of three.
|
@mhvk yes that should work too, I'll push that in a minute ! |
|
switching to draft for now, will rebase to edit the branch history and commit description when CI completes. |
mhvk
left a comment
There was a problem hiding this comment.
A few little leftovers, but looks good!
| np.concatenate(masks, out=out.mask, axis=axis) | ||
| np.concatenate(data, out=out.unmasked, axis=axis, dtype=dtype, casting=casting) | ||
| return out | ||
| return (out, None, None) |
There was a problem hiding this comment.
Might as well remove the parentheses if you rebase...
| for (result, mask) in zip(results, masks) | ||
| ) | ||
| return results if len(results) > 1 else results[0] | ||
| return results if len(results) > 1 else results[0], None, None |
There was a problem hiding this comment.
This does need parentheses,
return (results if len(results) > 1 else results[0]), None, None
|
|
||
| result = np.interp(xd, xp, fp, *args, **kwargs) | ||
| return result if xm is None else Masked(result, xm.copy()) | ||
| return result if xm is None else Masked(result, xm.copy()), None, None |
There was a problem hiding this comment.
Also here
return (result if xm is None else Masked(result, xm.copy())), None, None
cb8e920 to
80c7e11
Compare
…a (result, mask, out) tuple (fix compat with numpy dev)
80c7e11 to
13411bd
Compare
|
Should be good now 🤞🏻 |
mhvk
left a comment
There was a problem hiding this comment.
Super, looks good! Nice collaborative effort here!
…'s helper functions return a (result, mask, out) tuple (fix compat with numpy dev)
…948-on-v6.0.x Backport PR #15948 on branch v6.0.x (RFC: make all MaskedNDArray.__wrap_array__'s helper functions return a (result, mask, out) tuple (fix compat with numpy dev))
Description
Fixes #15947
This is a proof-of-concept level refactor, though maybe there's appetite to get this in as is to make devdeps CI green first.