8000 Make np.ma.take works on scalars by eric-wieser · Pull Request #7586 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

Make np.ma.take works on scalars #7586

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
May 1, 2016
Merged

Make np.ma.take works on scalars #7586

merged 2 commits into from
May 1, 2016

Conversation

eric-wieser
Copy link
Member

See #7585

@eric-wieser eric-wieser changed the title TST: Verify np.ma.take works on scalars Make np.ma.take works on scalars Apr 28, 2016
eric-wieser
eric-wieser reviewed Apr 28, 2016
View reviewed changes
numpy/ma/tests/test_core.py
@@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This raises the question of whether any of these tests are actually verifying things, since assert_equal will ignore the mask

@ahaldane
Copy link
Member

Nice catch!

I'm not sure I like how this always returns a 0-d array though. Eg, with this PR np.ma.array([1,2,3]).take(0) will return a 0-d array, unlike np.array([1,2,3]).take(0) which returns a scalar.

I think it might be better to detect the scalar case and return np.ma.masked if needed. Eg, leave most of take alone but add something like

if np.isscalar(out) and _mask[indices] is True:
    return masked
return out

(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)

@ahaldane
Copy link
Member
ahaldane commented Apr 29, 2016 •
edited
Loading

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 [...] and later converting back to scalar with [()].

@eric-wieser
Copy link
Member Author
eric-wieser commented Apr 29, 2016 •
edited
Loading

I'd argue that the version where we don't special case scalars is better - your fix introduces a bug when indexing with np.ma.masked.

Incidentally, np.isscalar(np.ma.masked) is False (#7588). Is this deliberate?

@ahaldane
Copy link
Member
ahaldane commented Apr 29, 2016 •
edited
Loading

That's true, you'd need to account for the indices is masked case.

One reason I prefer to avoid using [()] is it is buggy: see #7267. So take will not work for string and unicode types if you use it.

By the way, the reason np.isscalar(np.ma.masked) is False is related to the problem you noticed above, that scalars can't be viewed as ndarray subclasses: masked is an instance of MaskedArray which is a subclass of ndarray, but scalars are not subclasses of ndarrays.

These are all long-standing difficult problems people have thought about changing . For instance, in my opinion the very existence of np.ma.masked is a design mistake, and if/when we rewrite MaskedArray we should get rid of it completely. I know other numpy contributors also think we should get rid of scalars completely in favor of 0-d arrays, to avoid problems like this one.

(actually I think #7267 should be fixable without too much fuss, as I outlined in this comment, I just never got around to it)

@ahaldane
Copy link
Member
ahaldane commented Apr 29, 2016 •
edited
Loading

Sorry, I should have looked a little more before posting. Actually #7267 does not affect this PR since out is guaranteed to be an ndarray.

So actually I think your code is fine. It even does the right think if the user supplies a 0-d array as out.

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 [()]"?

@eric-wieser
BUG: Ensure mask is preserved on scalars
090bbdd 
By promoting and demoting between scalars and arrays where appropriate

See numpy#7585
@eric-wieser
Copy link
Member Author

Comments added

@ahaldane
Copy link
Member

Looks good! I'll merge i a little bit.

@charris charris added this to the 1.11.1 release milestone Apr 30, 2016
@ahaldane ahaldane merged commit ffb3112 into numpy:master May 1, 2016
@ahaldane
Copy link
Member
ahaldane commented May 1, 2016

Merged. Thanks @eric-wieser

@charris charris mentioned this pull request May 2, 2016
Closed
@charris charris removed this from the 1.11.1 release milestone May 2, 2016
@eric-wieser eric-wieser mentioned this pull request Mar 6, 2017
Open
@eric-wieser eric-wieser mentioned this pull request Jun 2, 2017
Open
@eric-wieser eric-wieser mentioned this pull request Jul 3, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
00 - Bug component: numpy.ma masked arrays
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eric-wieser @ahaldane @charris
0