BUG: fix np.ma.median with only one non-masked value and an axis argument #8030
Conversation
| @@ -736,7 +736,7 @@ def _median(a, axis=None, out=None, overwrite_input=False): | |||
| # duplicate high if odd number of elements so mean does nothing | |||
| odd = counts % 2 == 1 | |||
| if asorted.ndim > 1: | |||
| np.copyto(low, high, where=odd) | |||
| low[odd] = high[odd] | |||
There was a problem hiding this comment.
np.copyto(low, high, where=odd) doesn't unmask the values in contrary to low[odd] = high[odd].
There was a problem hiding this comment.
Now there are indexing errors because odd is boolean...
There was a problem hiding this comment.
Not sure I am following, do you have an example where things go wrong?
There was a problem hiding this comment.
There were problems when odd was a scalar or scalar array. Perhaps that no longer happens with the other changes...
There was a problem hiding this comment.
So now that we are using the count function, that may be fixed. In fact, by the time we get there 0-d inputs should have been raveled to 1-d and 1-d arrays have been handled, so the asorted.ndim > 1 check should not be needed either.
I don't think any of the values should be masked except for zero values of count, which brings up the question of negative indices in creating low.
There was a problem hiding this comment.
I am afraid I haven't got a grasp of the issue as good as yours, but what about replacing h - 1 by np.maximum(h - 1, 0) ?
There was a problem hiding this comment.
This way you can keep using np.copyto as previously.
There was a problem hiding this comment.
I think that should work also, I was just a bit concerned about speed. OTOH, it is a bit more obvious. Does is work?
There was a problem hiding this comment.
If we do that, can probably remove low._sharedmask = False also.
There was a problem hiding this comment.
I think that should work also, I was just a bit concerned about speed. OTOH, it is a bit more obvious. Does is work?
The tests pass on my machine, I pushed the change. Let's see what Travis and AppVeyor have to say!
If we do that, can probably remove low._sharedmask = False also.
OK I can try that too afterwards.
|
When (if) this passes the tests, would you squash the commits? |
and an axis argument. Add test.
2f06b05 to
ed30048
Compare
The tests were passing so I squashed the commits. BTW just in case you didn't know about it you can do squash + merge via the github UI these days. |
|
I don't really trust git to do it right after trying their revert button ;) Haven't tried squashing though. |
|
Thanks @lesteve. I still want to revisit |
|
Great!
If you have suggestions of weird edge cases to add I'd be happy to give it a go. |
|
If nothing else, there should be tests for different values of |
|
Thanks for the suggestions, I'll try to find some time to add more tests. |
Fix #8029.