Make np.ma.take works on scalars #7586
Conversation
| @@ -2998,6 +2998,10 @@ def test_take(self): | |||
| assert_equal(x.take([[0, 1], [0, 1]]), | |||
| masked_array([[10, 20], [10, 20]], [[0, 1], [0, 1]])) | |||
|
|
|||
| # assert_equal crashes when passed np.ma.mask | |||
There was a problem hiding this comment.
This raises the question of whether any of these tests are actually verifying things, since assert_equal will ignore the mask
1850dea to
6e5aa5e
Compare
|
Nice catch! I'm not sure I like how this always returns a 0-d array though. Eg, with this PR I think it might be better to detect the scalar case and return (As desired, this will not affect the case the user supplied out as a kwd, as in that case out will never be a scalar) |
|
Oh, sorry I misunderstood what is going on. I see that you don't return a 0-d array in the end. Still, maybe having an explicit if-statement is clearer than converting to 0-d with |
|
I'd argue that the version where we don't special case scalars is better - your fix introduces a bug when indexing with Incidentally, |
|
That's true, you'd need to account for the One reason I prefer to avoid using By the way, the reason These are all long-standing difficult problems people have thought about changing . For instance, in my opinion the very existence of (actually I think #7267 should be fixable without too much fuss, as I outlined in this comment, I just never got around to it) |
|
Sorry, I should have looked a little more before posting. Actually #7267 does not affect this PR since So actually I think your code is fine. It even does the right think if the user supplies a 0-d array as I would be happier with a comment though. Would you mind adding something to the effect of "account for scalar case by converting to 0-d with [...] and back with [()]"? |
By promoting and demoting between scalars and arrays where appropriate See numpy#7585
2e80d1b to
090bbdd
Compare
|
Comments added |
|
Looks good! I'll merge i a little bit. |
|
Merged. Thanks @eric-wieser |
See #7585