Skip to content

Fix indexing on ma.mvoid to be consistent with void; fixes #6724 #6755

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

gerritholl
Copy link
Contributor

Repair indexing on ma.mvoid so that it is consistent with void. Changes only behaviour currently broken. Fixes #6724.

Currently, or a standard ndarray, given Z = zeros((2,), dtype="(2,)i2,(2,)i2"), Z[0] is void but Z[0][0] and Z[0]["f1"] are ndarray. However, trying the same with ma.mvoid results in ValueError. With this commit, indexing a ma.mvoid with a similar dtype results in ma.masked_array.

@gerritholl gerritholl changed the title Fix indexing on ma.mvoid to be consistent with void; fixed #6724 Fix indexing on ma.mvoid to be consistent with void; fixes #6724 Dec 2, 2015
@gerritholl gerritholl force-pushed the structured_multidim_masked_array_mvoid branch 4 times, most recently from 574d209 to 5d69826 Compare December 2, 2015 12:51
…y.void

Make indexing on numpy.ma.mvoid consistent with indexing on numpy.void.
Does not change non-broken behaviour. Fixes numpy#6724.  Sometimes, indexing
ma.mvoid results in a non-scalar mask.  For example, dimension increases
if indexing with a multi-dimensional field.  Previously, this led to
a ValueError (truth value ambiguous).  With this commit, indexing now
returns an ma.masked_array so that there is no loss of information.

Note that there is a precedence for returning from void to array.  Z =
zeros((2,), dtype="(2,)i2,(2,)i2"), then Z[0] is a void, but Z[0][0]
and Z[0]["f1"] are array.  This commit therefore implements behaviouk
such that numpy.ma.mvoid is consistent with numpy.void.

Also adds a related test.
@gerritholl gerritholl force-pushed the structured_multidim_masked_array_mvoid branch from 5d69826 to f36db77 Compare December 2, 2015 12:53
@mhvk
Copy link
Contributor

mhvk commented Dec 2, 2015

Had to wrap my head around this one, but agree you are making it do the right thing.

@@ -5869,6 +5869,18 @@ def __getitem__(self, indx):

"""
m = self._mask
if m[indx].size > 1:
Copy link
Member

Choose a reason for hiding this comment

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

It is possible to have subarrays of size 1, eg dtype(["A", "i2", (1,1,1)]) so this probably needs something better. m[indx].shape == () was an idea, but that won't work for object types. Probably you just have to check isinstance(m[indx], np.ndarray).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ahaldane True, my reasoning was that the status quo does not fail for subarrays of size 1. So there is a trade-off between changing existing behaviour, or being consistent with new behaviour.

Copy link
Member

Choose a reason for hiding this comment

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

Now that I've tested I see your point.

But what if we get really crazy with a subarray of size 1 of structured dtype:

>>> a = np.ma.array(np.ones(2, dtype=[('A', 'i4,i4', (1,1,1)), ('B', 'i4')]), 
                    mask=[([[[(0, 0)]]], 0), ([[[(0, 0)]]], 0)])
>>> a[0]['A']
masked

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It appears that crazier dtypes do not get assigned to the mask properly anyway:

In [234]: Z = ma.zeros(2, dtype([("A", "(2,2)i1,(2,2)i1", (2,2))]))

In [237]: print(Z.dtype.descr)
[('A', [('f0', '|i1', (2, 2)), ('f1', '|i1', (2, 2))], (2, 2))]

In [238]: print(Z.mask.dtype.descr)
[('A', '|b1', (2, 2))]

That's bound to lead to problems somewhere as well...

@charris
Copy link
Member

charris commented Dec 2, 2015

@gerritholl I'm going to trust you and @mhvk on this one unless @ahaldane wants to do the honors.

@gerritholl
Copy link
Contributor Author

If I find time tonight I will think again about my alternative vs. ahaldanes alternative, I might not have time before tomorrow night though.

@ahaldane
Copy link
Member

ahaldane commented Dec 2, 2015

I haven't had a chance to test it yet because of build issues! But I will soon.

@charris
Copy link
Member

charris commented Dec 3, 2015

Closing in favor of #6763, this can be reopened if discussion warrants it.

@charris charris closed this Dec 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants