Skip to content

BUG Ensure NotImplemented is passed on in MaskedArray ufunc's #3900

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

Merged
merged 3 commits into from
Oct 13, 2013

Conversation

mhvk
Copy link
Contributor

@mhvk mhvk commented Oct 12, 2013

Currently, masked array ufunc replacements do not check whether the ufunc call on the data proper actually works (see below). This PR ensures that they pass on NotImplemented if needed, so that classes more interesting than the string below will have a chance to try to do the reverse operation.

Note that this is a long-standing bug; I'm not sure how far it can be backported, but hope it can make 1.8.0.

p.s. The reason I stumbled upon this is that I am trying to implement a MaskedQuantity class for astropy [1]. For this package, we already have Unit and Quantity classes, and set it up such that array * unit returns a Quantity; I'd hope to do have masked_array * unit similarly to return a MaskedQuantity.

[1] http://docs.astropy.org/en/latest/units/quantity.html


In [6]: np.array([1.,2.]).__mul__('abc')
Out[6]: NotImplemented

In [7]: np.ma.MaskedArray([1.,2.]).__mul__('abc')
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-7-bc634e0e858e> in <module>()
----> 1 np.ma.MaskedArray([1.,2.]).__mul__('abc')

/usr/lib/pymodules/python2.7/numpy/ma/core.pyc in __mul__(self, other)
   3646     def __mul__(self, other):
   3647         "Multiply other by self, and return a new masked array."
-> 3648         return multiply(self, other)
   3649     #
   3650     def __rmul__(self, other):

/usr/lib/pymodules/python2.7/numpy/ma/core.pyc in __call__(self, a, b, *args, **kwargs)
    938             np.seterr(**err_status_ini)
    939         # Case 1. : scalar
--> 940         if not result.ndim:
    941             if m:
    942                 return masked

AttributeError: 'NotImplementedType' object has no attribute 'ndim'

@pv
Copy link
Member

pv commented Oct 12, 2013

Needs tests.

@njsmith
Copy link
Member

njsmith commented Oct 12, 2013

Needs tests.
On 12 Oct 2013 17:26, "Marten van Kerkwijk" notifications@github.com
wrote:

Currently, masked array ufunc replacements do not check whether the ufunccall on the data proper actually works (see below). This PR ensures that
they pass on NotImplemented if needed, so that classes more interesting
than the string below will have a chance to try to do the reverse
operation.

Note that this is a long-standing bug; I'm not sure how far it can be
backported, but hope it can make 1.8.0.

p.s. The reason I stumbled upon this is that I am trying to implement a
MaskedQuantity class for astropy [1]. For this package, we already have
Unit and Quantity classes, and set it up such that array * unit returns a
Quantity; I'd hope to do have masked_array * unit similarly to return a
MaskedQuantity.

[1] http://docs.astropy.org/en/latest/units/quantity.html

In [6]: np.array([1.,2.]).mul('abc')
Out[6]: NotImplemented

In [7]: np.ma.MaskedArray([1.,2.]).mul('abc')

AttributeError Traceback (most recent call last)
in ()
----> 1 np.ma.MaskedArray([1.,2.]).mul('abc')

/usr/lib/pymodules/python2.7/numpy/ma/core.pyc in mul(self, other)
3646 def mul(self, other):
3647 "Multiply other by self, and return a new masked array."
-> 3648 return multiply(self, other)
3649 #
3650 def rmul(self, other):

/usr/lib/pymodules/python2.7/numpy/ma/core.pyc in call(self, a, b, _args, *_kwargs)
938 np.seterr(**err_status_ini)
939 # Case 1. : scalar
--> 940 if not result.ndim:
941 if m:
942 return masked

AttributeError: 'NotImplementedType' object has no attribute 'ndim'


You can merge this Pull Request by running

git pull https://github.com/mhvk/numpy ma/pass-on-NotImplemented

Or view, comment on, or merge it at:

#3900
Commit Summary

  • Ensure NotImplemented is passed on in MaskedArray ufunc's

File Changes

Patch Links:

@mhvk
Copy link
Contributor Author

mhvk commented Oct 12, 2013

Needs tests.

not having contributed to numpy before, I'm not familiar with its
testing framework; should I simply add to a suitable file in the tests
subdirectory? Also, if you have it handy, could you give me a pointer to
how to set up a testing environment locally? Thanks!

Prof. M. H. van Kerkwijk
Dept. of Astronomy & Astroph., 50 St George St., Toronto, ON, M5S 3H4, Canada
McLennan Labs, room 1203B, tel: +1(416)9467288, fax: +1(416)9467287
mhvk@astro.utoronto.ca, http://www.astro.utoronto.ca/~mhvk

@charris
Copy link
Member

charris commented Oct 12, 2013

All you need to do is install nose. After that the easiest way to proceed is python runtests.py --help, which will tell you what to do. As to the test itself, put it in numpy/ma/test/test_core.py; use the other tests there as templates. Note that the name of the test needs to start with test_.

@mhvk
Copy link
Contributor Author

mhvk commented Oct 12, 2013

@charris - thanks, that probably saved me hours! Now added test cases.

@@ -1662,6 +1662,26 @@ def test_ndarray_mask(self):
assert_equal(test.mask, control.mask)
self.assertTrue(not isinstance(test.mask, MaskedArray))

def test_treatment_of_NotImplemented(self):
"Check we return NotImplemented if ufunc cannot deal with other"
Copy link
Member

Choose a reason for hiding this comment

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

For nose related reasons, don't use docstrings in tests, just do # ...

@charris
Copy link
Member

charris commented Oct 12, 2013

Looking good, just a few issues.

@charris
Copy link
Member

charris commented Oct 12, 2013

This will need a backport to both 1.8.x and 1.7.x.

@mhvk
Copy link
Contributor Author

mhvk commented Oct 12, 2013

@charris - OK on all except the docstring -- since this would break consistency with what is done in the rest of test_core.py. If this is really better not done, I could change all the functions in a separate PR.

@mhvk
Copy link
Contributor Author

mhvk commented Oct 12, 2013

@charris - on the backport, do I need to do anything for that?

@charris
Copy link
Member

charris commented Oct 13, 2013

It's probably easier for me to do the backports unless you want the experience. The docstring thing isn't a major point and the masked arrays haven't seen much maintenance in years. If you fixup the rest of the functions in another PR, that would be great and I'll go ahead and put this in when Travis passes.

@charris
Copy link
Member

charris commented Oct 13, 2013

In it goes, thanks.

If you fix the docstrings it might be worth doing a PEP8 cleanup at the same time. I don't know how conformant the file is at the moment.

charris added a commit that referenced this pull request Oct 13, 2013
BUG Ensure NotImplemented is passed on in MaskedArray ufunc's
@charris charris merged commit 46b1a4c into numpy:master Oct 13, 2013
This was referenced Oct 13, 2013
@mhvk mhvk deleted the ma/pass-on-NotImplemented branch October 13, 2013 02:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants