BUG: fix calling np.nanvar and np.nanstd with Quantity inputs + an out argument#17354
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 |
b5fc1b0 to
6cac14d
Compare
6cac14d to
e553767
Compare
There was a problem hiding this comment.
Darn, silly to miss the out argument. See inline comments about just ignoring the unit of out, since it can be overwritten.
| return (a.view(np.ndarray),) + args, kwargs, unit, None | ||
|
|
||
|
|
||
| def _nanop_helper(a, *, out, _out_unit_from_a): |
There was a problem hiding this comment.
Hmm, this is not how ufuncs treat out, so will need a bit of adjustment. Basically, if out is given, the unit should just be overwritten (just like the data are), so out_unit is just the unit of a, or dimensionless if a is not a Quantity (which is possible; numpy functions dispatch on both input and output arrays).
There was a problem hiding this comment.
numpy functions dispatch on both input and output arrays
this is actually why I needed to factor in out's units: if not, calling np.nanvar(q, out=o) (where q and o are Quantitys) would first be processed nanvar's helper (which strips out q's unit) and then through nanstd's helper (because o is still a quantity), but then o is the only place where the output unit is still available.
This seems critical so I want to find an agreement on this point before I move on to your other suggestions.
| # TODO: for an ndarray output, we could in principle | ||
| # try converting all Quantity to dimensionless. | ||
| raise NotImplementedError | ||
| out_unit = out.unit |
There was a problem hiding this comment.
So I think this check goes to the detailed code, just using getattr(a, "unit", u.one)
| if out_unit is None: | ||
| out_unit = _out_unit_from_a(a) | ||
| else: | ||
| assert out_unit == _out_unit_from_a(a) |
There was a problem hiding this comment.
As noted, out's unit can be overwritten and a can be a plain ndarray.
| return out_kwarg, out_unit | ||
|
|
||
|
|
||
| @function_helper |
There was a problem hiding this comment.
I'm not sure nanstd actually needs a helper...
e553767 to
e276f5f
Compare
|
@neutrinoceros - I had to try myself to see what the problem was and ended up fixing it locally - and felt it was easiest to just force-push your branch, sorry! The tests now also include one for array input with quantity output. |
There was a problem hiding this comment.
Approving, though since I changed @neutrinoceros's code, best to get him to approve too.
| # Also check array input, Quantity output. | ||
| o2 = np.nanstd(self.q.value, out=out) | ||
| assert o2 is out | ||
| assert o2.unit == u.dimensionless_unscaled |
There was a problem hiding this comment.
this makes me wonder, shouldn't we also check the unit in o/out in the first half of the test ? (same question for test_nanvar_out)
There was a problem hiding this comment.
It really is a bit of overkill, since just checking equality checks the unit too (and I chose it to be initialized with the wrong one one purpose). Here, though, I felt that with an array input it made perhaps more sense.
But obviously feel free to add the unit check!
There was a problem hiding this comment.
I chose it to be initialized with the wrong one one purpose
this wasn't obvious to me just from reading the test and I can easily imagine it looking like an oversight or a mistake to some future reader, so I expanded the tests with parametrization and added a comment to make this intention very explicit.
There was a problem hiding this comment.
OK, I think this covers the bases! Let's get it in.
e276f5f to
e96dc43
Compare
|
I revised the test again, so I feel like @mhvk should give it one (last ?) look before we merge. |
…d` with `Quantity` inputs + an `out` argument
…d` with `Quantity` inputs + an `out` argument
…354-on-v7.0.x Backport PR #17354 on branch v7.0.x (BUG: fix calling `np.nanvar` and `np.nanstd` with `Quantity` inputs + an `out` argument)
…354-on-v6.1.x Backport PR #17354 on branch v6.1.x (BUG: fix calling `np.nanvar` and `np.nanstd` with `Quantity` inputs + an `out` argument)
Description
Fixes #17353
Given this was a regression in astropy 6.1.5, I would be tempted to propose a hotfix 6.1.6 release, so I'm requesting a review from the release team.