BUG: Masked arrays and apply_over_axes #4463
Conversation
| @@ -416,24 +416,23 @@ def apply_over_axes(func, a, axes): | |||
| """ | |||
| (This docstring will be overwritten) | |||
| """ | |||
| val = np.asarray(a) | |||
| msk = getmaskarray(a) | |||
| val = asarray(a) | |||
| N = a.ndim | |||
| if array(axes).ndim == 0: | |||
| axes = (axes,) | |||
| for axis in axes: | |||
| if axis < 0: axis = N + axis | |||
There was a problem hiding this comment.
Please clean that up a bit by putting axis = N + axis on the next line. I know it wasn't yours to begin with ;)
There was a problem hiding this comment.
I copied the entire body of this function from np.apply_over_axes. I don't mind cleaning it up a bit, but I don't want to introduce spurious differences between the two functions. Should I reformat np.apply_over_axes as well?
There was a problem hiding this comment.
Sure, make it a different commit though.The message prefix would be STY:.
|
I looks like |
| N = a.ndim | ||
| if array(axes).ndim == 0: | ||
| axes = (axes,) | ||
| for axis in axes: | ||
| if axis < 0: axis = N + axis | ||
| args = (val, axis) | ||
| res = ma.array(func(*(val, axis)), mask=func(*(msk, axis))) | ||
| res = func(*args) |
There was a problem hiding this comment.
This is another place that could use a clean-up. I would simply write func(val, axis) in the line above.
There was a problem hiding this comment.
What about the mask=func(*(msk, axis))? That is what concerned me. The original does something with the mask, the new version doesn't.
There was a problem hiding this comment.
This made no sense to me. And neither did f(*(a, b)) instead of f(a, b). Looks like a bit of overengineering.
ma.apply_over_axes inherits docstring from np.apply_over_axes, so it should work the same way and delegate mask handling to func. This is what the OP issue was about.
There was a problem hiding this comment.
OK. Agree about the f(*(a, b)) function call. Can that be cleaned up without too much trouble? Also, even though the docstring is currently inherited, it might make sense to have one where you can mention that the mask handling is left to func.
|
@abalkin Can you finish this up? I'd like to put it in. |
| raise ValueError("Function is not returning"\ | ||
| " an array of correct shape") | ||
| raise ValueError("function is not returning " | ||
| "an array of the correct shape") |
There was a problem hiding this comment.
PEP8, but this will get fixed later when we pepify these files.
There was a problem hiding this comment.
This was copied from np.apply_over_axes. I assume "pepify" will fix this in both places.
There was a problem hiding this comment.
In particular, the indent should be
raise ValueError("function is not returning"
" an array of the correct shape")
or
raise ValueError(
"function is not returning an array of the correct shape")
|
@abalkin Be good to get this finished up. |
Masked arrays version of apply_over_axes did not apply function correctly to arrays with non-trivial masks. Fixes numpy#4461.
|
@charris - this should be good to go. |
|
In it goes. We'll see if this change causes any problems. Thanks @abalkin. |
BUG: Masked arrays and apply_over_axes
|
Looks like this broke something: gh-4627 |
|
@abalkin I'd guess this comes from not applying the function to the mask. |
|
@charris - can you demonstrate the regression in a simple case? I am not familiar with |
|
Found the issue: it's because The new behavior is arguably better, but this PR may still silently break working code. Therefore I think we should restore the copy behavior. |
|
@rgommers - does |
|
It looks like |
Masked arrays version of apply_over_axes did not apply
function correctly to arrays with non-trivial masks.
Fixes #4461.