-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
BUG: Masked arrays and apply_over_axes #4463
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PEP8, but this will get fixed later when we pepify these files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was copied from np.apply_over_axes. I assume "pepify" will fix this in both places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.