BUG Ensure masked object arrays can always return single items. #5962

mhvk · Jun 12, 2015

For a masked array holding objects that themselves are arrays, when selecting a single item, it is treated as if it were a slice of the array and an attempt is made to set its mask. This was always a bug, but it become visible with a recent change to MaskedArray.__getitem__ (#4586) where it is attempted to change the shape of the mask (to help matrices, sigh, was not even part of PR initially!). With this PR, the treatment is the same as for other single items: one either gets the array stored in the object, or masked.

Test case that now works but used to fail:

mx1 = MaskedArray([1.], mask=[True])
mx2 = MaskedArray_array([1., 2.])
mx = MaskedArray([mx1, mx2], mask=[False, True])
mx[0]

mhvk · Jun 12, 2015

p.s. Bug found by testing astropy -- see astropy/astropy#3848. It seems our test suite checks even the dark corners of numpy!

embray · Jun 12, 2015

Would not be the first time we've tested out dark corners of Numpy :)

charris · Jun 12, 2015

Probably a case where a little light pollution would be a good thing ;)

seberg · Jun 12, 2015

Just face it, you guys are basically the MA maintainers now ;)

mhvk · Jun 12, 2015

Thanks but no thanks ;-) It does look like travis passes; I think this is fine, but is there anyone else that should have a look?

ahaldane · Jun 12, 2015

numpy/ma/core.py

            # A record ................
            if isinstance(dout, np.void):
-                mask = _mask[indx]
+                mask = nomask if _mask is nomask else _mask[indx]


This line is a second bugfix, unrelated to object arrays, right?

That's right -- did that in the process and forgot about it...

I'll add a test...

(BTW, shall I volunteer you for maintainer........)

Actually, it seems one cannot initialise a MaskedArray with record type and nomask, so I'll just get rid of that change.

mhvk · Jun 12, 2015

@ahaldane - thanks for checking! The extraneous change is now removed.

ahaldane · Jun 12, 2015

I'm going to have to pass on the maintainership :)

The PR looks good now.

mhvk · Jun 14, 2015

@ahaldane - it turns out the fix I had was not enough: at least in astropy there are tests that rely on the previous buggy behaviour where the array held in the masked object array was returned as a MaskedArray that, e.g., had zero length (i.e., tests were done like len(mx[0]) == 0). I don't think this corner case is important enough to break backwards compatibility that much, so I now made the test specifically for object arrays, returning a fully masked array for this case (as before, but not doing casting to type(self) or doing update_from(self), which is silly if one gets a single element).

At least all the tests in astropy now pass that previously failed because of my PR (but it seems some recent change in recarray has broken some of our io.fits tests... yikes).

ahaldane · Jun 14, 2015

That's worrying.. which tests? I just tried building+testing astropy with this PR, and I don't see anything (8753 passed, 199 skipped, 43 xfailed, 2 xpassed )

mhvk · Jun 17, 2015

@ahaldane - there definitely failures in astropy.io.fits (which I hope will be resolved by #5943). In addition, the removal of NotImplemented in np.ufunc (#5964, #5864) brought forth a mistake in Quantity which causes failures in astropy.units (see astropy/astropy#3860).

None of these errors are related to the ones solved by this PR, though.

EDIT: sadly, #5943 does not resolve the astropy.io.fits problems.

charris · Jun 17, 2015

@mhvk Fixing the astropy.io.fits problem looks like a 1.10 blocker. Could you spend some time on that?

ahaldane · Jun 17, 2015

@mhvk I'm happy to help debug this, if you know which tests fail.

astropy.io.fits passed all tests in python2 on my (linux) system with numpy master. It failed a bunch in python3, but it fails even if I use a numpy revision before any of my recent recarray commits.

mhvk · Jun 17, 2015

hmm, so a python3 issue for io.fits.. Odd. I'll try to look later. In the meantime, CC @embray.

mhvk · Jun 18, 2015

@charris - still investigating the io.fits issue (now clearly related to python3). Hopefully solved soon. But as for importance: this present PR is actually more important, as it prevents the bug I exposed in #4586 from breaking many of our tests.

charris · Jun 18, 2015

@mhvk I agree that this is a bug fix len(mx[0]) == 0 is just weird for a not empty object. Even so, it should probably be mentioned in the release notes. I assume mx2 = MaskedArray_array([1., 2.]) should be mx2 = MaskedArray(array([1., 2.])) in the example.

For a masked array holding objects that themselves are arrays, when selecting a single item, it is treated as if it were a slice of the array and an attempt is made to set its mask. This was always a bug, but it become visible with a recent change to `MaskedArray.__getitem__` (numpygh-4586) where it is attempted to change the shape of the mask. With this PR, this case gets special treatment in that the object is made into a Masked Array with a full set mask. (A previous attempt to do the perhaps more logical think and just return `masked` caused quite a few errors in astropy.io.votable; it seemed it was not worth breaking backwards compatibility that much). Test case that now works but used to fail: ``` mx1 = MaskedArray([1.], mask=[True]) mx2 = MaskedArray([1., 2.]) mx = MaskedArray([mx1, mx2], mask=[False, True]) mx[0] ```

mhvk · Jun 18, 2015

OK, I added a brief item to the release notes, and corrected my commit message. Let me know if this makes sense.

BUG Ensure masked object arrays can always return single items.

charris · Jun 18, 2015

Thanks @mhvk.

eric-wieser · Apr 19, 2017

This PR introduced a different bug - #8906. Do we need to keep this behaviour of not just returning np.ma.maskedin this case?

mhvk mentioned this pull request Jun 12, 2015

Numpy-dev issues for votable astropy/astropy#3848

Closed

ahaldane reviewed Jun 12, 2015
View reviewed changes

mhvk force-pushed the ma-getitem-nomask-correction branch from 6bd731b to c4c2eee Compare June 12, 2015 19:01

charris added 00 - Bug component: numpy.ma masked arrays labels Jun 13, 2015

mhvk force-pushed the ma-getitem-nomask-correction branch from c4c2eee to 84fa8d9 Compare June 14, 2015 00:46

mhvk force-pushed the ma-getitem-nomask-correction branch 2 times, most recently from c1cfd7e to 6049392 Compare June 14, 2015 01:24

mhvk mentioned this pull request Jun 17, 2015

Numpy 1.10 issues for io.fits astropy/astropy#3854

Closed

mhvk force-pushed the ma-getitem-nomask-correction branch from 6049392 to 0db6200 Compare June 18, 2015 00:18

mhvk force-pushed the ma-getitem-nomask-correction branch from 0db6200 to 1a4d943 Compare June 18, 2015 02:17

mhvk mentioned this pull request Jun 18, 2015

Quantity updates for numpy dev astropy/astropy#3860

Merged

charris added a commit that referenced this pull request Jun 18, 2015

Merge pull request #5962 from mhvk/ma-getitem-nomask-correction

2c29b13

BUG Ensure masked object arrays can always return single items.

charris merged commit 2c29b13 into numpy:master Jun 18, 2015

mhvk deleted the ma-getitem-nomask-correction branch June 18, 2015 13:07

eric-wieser mentioned this pull request Apr 19, 2017

BUG: Masked object array of ndarrays attaches mask to masked array value #8906

Open

eric-wieser mentioned this pull request Apr 19, 2017

BUG: Correctly distinguish between 0d arrays and scalars in MaskedArray.__getitem__ #8905

Merged

fengluoqiuwu mentioned this pull request Oct 26, 2024

Object-type masked array special-cases elements that are ndarray #18981

Open

Search code, repositories, users, issues, pull requests...

Uh oh!

BUG Ensure masked object arrays can always return single items. #5962

BUG Ensure masked object arrays can always return single items. #5962

Uh oh!

Conversation

mhvk commented Jun 12, 2015

Uh oh!

mhvk commented Jun 12, 2015

Uh oh!

embray commented Jun 12, 2015

Uh oh!

charris commented Jun 12, 2015

Uh oh!

seberg commented Jun 12, 2015

Uh oh!

mhvk commented Jun 12, 2015

Uh oh!

ahaldane Jun 12, 2015

Choose a reason for hiding this comment

Uh oh!

mhvk Jun 12, 2015

Choose a reason for hiding this comment

Uh oh!

mhvk Jun 12, 2015

Choose a reason for hiding this comment

Uh oh!

mhvk commented Jun 12, 2015

Uh oh!

ahaldane commented Jun 12, 2015

Uh oh!

mhvk commented Jun 14, 2015

Uh oh!

ahaldane commented Jun 14, 2015

Uh oh!

mhvk commented Jun 17, 2015

Uh oh!

charris commented Jun 17, 2015

Uh oh!

ahaldane commented Jun 17, 2015

Uh oh!

mhvk commented Jun 17, 2015

Uh oh!

mhvk commented Jun 18, 2015

Uh oh!

charris commented Jun 18, 2015

Uh oh!

mhvk commented Jun 18, 2015

Uh oh!

charris commented Jun 18, 2015

Uh oh!

eric-wieser commented Apr 19, 2017

Uh oh!

Uh oh!