I'm a little wary of putting in a change of this complexity into 1.8 at
this point - there's lots of room for unexpected consequences here...
On 13 Oct 2013 07:04, "Marten van Kerkwijk" notifications@github.com
wrote:

This is a follow-on to #3900 #3900,
making two furthe changes to tryto reach the ultimate goal of MaskedArraybehaving sensibly for subclasses.

1. Ensure that for two-argument ufunc's, the data in the subclass are no
   longer converted to ndarray; rather, a possible subclass is kept. This is
   particularly relevant for my work on a MaskedQuantity class, where _datacontains a
   Quantity which has not just a value (the ndarray) but also a unit.
2. Avoid the creation of object arrays, so that non-array Classes with an
   array_priority set and with a reverse method defined properly lead to
   NotImplemented (i.e., making the behaviour match that of regular arrays).

In order for this to work, I

• removed subok=False in the getdata calls in the two BinaryOperationclasses
• slightly changed the method used to keep previous values when they
  are masked, including allowing this to fail (e.g., when I multiply two
  masked quantities in meters, the result is an area, and hence it is
  senseless to keep the original values, and quantities raise an appropriate
  UnitError). (Without this, the matrix subclass tests would not pass).
• slightly changed the order, with result masks only calculated if the
  primary calculation succeeded.
• slightly changed getdata to avoid the creation of object arrays, and
  getmaskedarray to deal with the consequences.

I also added a test case, which perhaps best makes clear why this is
useful.

CC @charris https://github.com/charris -- I consider the above a
correction of a bug, but realise this is subjective; it could also be ENH,
even though the API does not change. It would be wonderful if this could

still make it to 1.8.

You can merge this Pull Request by running

git pull https://github.com/mhvk/numpy ma/allow-subclass-in-ufunc

Or view, comment on, or merge it at:

#3907
Commit Summary

• Allow subclasses in MaskedArray ufuncs -- for non-ndarray _data

File Changes

• M numpy/ma/core.pyhttps://github.com/BUG allow subclasses in MaskedArray ufuncs -- for non-ndarray _data #3907/files#diff-0(76)
• M numpy/ma/tests/test_core.pyhttps://github.com/BUG allow subclasses in MaskedArray ufuncs -- for non-ndarray _data #3907/files#diff-1(23)

Patch Links:

• https://github.com/numpy/numpy/pull/3907.patch
• https://github.com/numpy/numpy/pull/3907.diff

Proposed changes sound good to me.

Tested with scipy.stats.mstats, no issues.

Test failure is real, needs fixing.

I think it is too late for 1.8.0. However, it is good that masked arrays are getting a look over. There may be other parts that can be improved or fixed up using some of the machinery that has gone into numpy in the last few years. In particular, I'm wondering if the new __numpy_ufunc__ functionality in 1.9 can be used.

@njsmith, @rgommers, @charris - having spent several hours looking at the test failure, I now agree this PR is not for 1.8... The failure is actually a combination of two previous bugs canceling each other out (see below).
I made a separate PR to correct that (#3914); I also included that in this PR, so that the tests all pass again

The test that fails effectively does

import numpy as np; from numpy import isclose, nan, isnan
x = np.ma.masked_where([True, True, False], [nan, nan, nan])
y = isclose(nan, x, equal_nan=True)
# and at the very end of `isclose` (filling in vars),
cond = np.zeros_like(x, dtype=np.bool)  # net effect of quite a few lines
cond[isnan(x) & isnan(nan*np.ones_like(x, dtype=np.bool))] = True
assert all(cond._mask == [True, True, False])

Now the differences are:

1. (nan*np.ones_like(x, dtype=np.bool))._data yields array([ 0., 0., nan]) in the current version and array([ nan, nan, nan]) in my PR. Of course, in both cases the first two elements are masked.
2. Since a mask on the index for is ignored in __setitem__, and since for any items set also the mask is adjusted, the assignment to cond[isnan...] = True] by chance keeps the mask OK in the current version, and clears it for all elements for mine.

The basic problem is not so much what is done to masked values (FWIW, I think my PR improves that), but rather that masked values should not be used in the assignment. Indeed, there is a commented out check on the index in MaskedArray.__setitem__ -- but turning that on finds a number of other test failures... And while it should not be used, it is also good if it can be made to work as frequently as possible (#3914 attempts to do so).

While writing, may as well ask: ideally, I'd like to see that, e.g., masked_array * array_subclass returns masked_array_subclass if such a subclass is available (in my particular case, MaskedQuantity). In principle, this is not too difficult if the array subclass has a link to a masked version of itself, say an _masked_class attribute). Would something like that make sense? Is it fine to introduce such a standard? Or is there perhaps something in place already? (E.g., is one perhaps meant to use update_from? I'm a bit confused by that).

Looks like you merged master into this. That is generally not a good idea, better to rebase on master, and I think you should do that here: git rebase master. That should clean things up a bit. If things get really messed up, the other option is a new branch off current master and merge this, but I haven't had to do that.

"Is it fine to introduce such a standard?" Questions like this are best raised on the list for discussion.

@charris - OK, rebased (and I'll raise my question on the mailing list)

Ping Travis.

@charris - I had somewhat forgotten about this PR, and looking back at it realised the main thing still missing was documentation of the change in behaviour for subok=False in getdata for the case where the input is not an array (sub)class, nor anything easily convertible into an array. I've now added that. The other issue we discussed above was whether self.__rmul__(other) should lead to multiply(other, self) or the reverse (and same for __radd__); I think by analogy with __rdiv__ (and __rsub__) my approach is slightly more logical, but as @pv mentioned, any code that relies on either behaviour is badly written. I'm certainly fine with taking that change out.

@mhvk 1.9 is coming up, anything you want to add?

@charris - thanks for warning me about 1.9. I think this is still a very useful change. Just to be sure it is still OK, I rebased on current master and pushed it again (assuming this triggers a new travis built).

It so happens I just started working again on getting masked subclasses to work in astropy, with the current idea being that we'll have a MaskedArray subclass in which we override parts that currently do not work well, and make separate PRs to numpy for those. I'll try to get the simpler ones of those in soon.

@charris - an astropy bug report (astropy/astropy#2701) reminded me of this solution. I'm assuming it is too late to get this in 1.9 still; if not, do let me know and I'll rebase & ensure it is mergeable.

@mhvk Needs a rebase. And the commit messages could use a tweak, see doc/source/dev/gitwash/development_workflow.rst for prefixes.

I've been putting off all the uncommited fixes/workarounds from astropy until 1.10 development starts. This doesn't look terribly risky, but at this point I'd prefer all those changes have more time to settle.

@charris - fair enough. I rebased it anyway, even though as I think you wrote earlier, it may well be best to rewrite MaskedArray using __numpy_ufunc__, in which case all of this may be irrelevant. Also, for astropy, since we have to support lower versions of numpy, we'll anyway be stuck with half-forking ma.py (of course, I'll continue to raise issues we find; I feel slightly guilty for being responsible for two out of the three remaining 1.9 blockers...)

As an aside, you might be interested in the thread that contains this quote

Aside: While I am at it, let me reiterate what I have said to the other
developers privately: there is NO value to inheriting from the array
class. Don't try to achieve that capability if it costs anything, even
just effort, because it buys you nothing. Those of you who keep
remarking on this as if it would simply haven't thought it through IMHO.
It sounds so intellectually appealing that David Ascher and I had a
version of Numeric that almost did it before we realized our folly.

@charris - thanks for the quote! Though I should add that after my involvement in getting Quantity to work properly as an ndarray subclass, I would disagree. It may sound trivial, but it helps enormously to simply be able to do np.sin(q) where q could be an ndarray in radians or a quantity with any angular unit, if only because code can be reused so much more easily than if one had to import Quantity's own ufuncs. Unfortunately, MaskedArray is a bit less friendly to work with...

@charris - thanks for pointing me to #5227. I tried the trial implementation of taking account of __array_priority__ of @abalkin in #5230 and that solves the problem where <masked-array> * <unit> made a masked array of object dtype. With that, I think that the somewhat problematic edits to getdata would no longer be needed.

However, #5230 does not address that subclasses should be passed on (the main goal of this PR). Since it is simpler, though, maybe it should be done first?

@mhvk Where do you think this stands with respect to the __numpy_ufunc__ discussion? Also, needs a fix if this is going to be considered for 1.10.

@charris - This is in addition to the discussion on __array_priority__ -- ideally, I would rebase after #5864 is merged, and focus just on the subclass issue (i.e., remove the changes to getdata).

@mhvk Thanks for the update. I'll let this wait on #5864 then.

Could use a couple of comments.

@mhvk #5864 is in.

@charris - thanks, I'll try to look at this and possible tests for #4617 as soon as I can.

@charris - OK, I rebased this one, which meant that, as hoped, the changes to getdata could be reverted. Furthermore, with #5864 in, there no longer is a need to pass down NotImplemented from within the ufunc implementations (more in the spirit of what we seem to be getting to for #5844, so good in that sense). So, all that's left is now a rewrite that ensures that subclasses get to tell how the ufuncs are done. Which is quite clean and nicely stand-alone.

Apart from that, I added the comments to __radd__ and __rmul__ you requested and one test for #4617 (can do more if you think it is important but I ran out of time today).

Hmm, this test failure looks potentially real (but maybe not repeatable). I'm going to restart the test but keeping this for reference. The numpy/ma/testutils.py is the only thing that looks related.

FAIL: test_io.test_load_refcount

----------------------------------------------------------------------

Traceback (most recent call last):

  File "/home/travis/build/numpy/numpy/builds/venv/lib/python2.7/site-packages/nose/case.py", line 197, in runTest

    self.test(*self.arg)

  File "/home/travis/build/numpy/numpy/builds/venv/lib/python2.7/site-packages/numpy/lib/tests/test_io.py", line 1920, in test_load_refcount

    assert_equal(n_before, n_after)

  File "/home/travis/build/numpy/numpy/builds/venv/lib/python2.7/site-packages/numpy/ma/testutils.py", line 100, in assert_equal

    raise AssertionError(msg)

AssertionError: 

Items are not equal:

 ACTUAL: 55930

 DESIRED: 55911

@charris - yes, I had seen that very weird test result, and was going to ask to restart, as it seemed unlikely any change in the python code would lead to a refcount problem. But you beat me to it... oddly, it seems irreproduceable (though at least it means it was not due to this PR).

Well, lets get this in. Thanks @mhvk.

A possibly related issue on the mailing list: https://mail.scipy.org/pipermail/numpy-discussion/2016-February/074940.html