Skip to content

BUG: Fix bugs for np.ma.count and copy on builtin types (issue #8019)#8060

Closed
skwbc wants to merge 3 commits intonumpy:masterfrom
skwbc:issue#8019
Closed

BUG: Fix bugs for np.ma.count and copy on builtin types (issue #8019)#8060
skwbc wants to merge 3 commits intonumpy:masterfrom
skwbc:issue#8019

Conversation

@skwbc
Copy link
Contributor

@skwbc skwbc commented Sep 18, 2016

As explained in issue #8019 list has a count (in Python3) and copy method, and _frommethod.__call__ tries to call them instead of np.ma.MaskedArray.count and np.ma.MaskedArray.copy.
By using methods on MaskedArray always, we can resolve this issue (unexpected call of builtin object methods).
I think this fix is fine because docstring on _frommethod says “Define functions from existing MaskedArray methods”.

@skwbc skwbc changed the title BUG: Fix bugs for np.ma.count and copy on builtin types (issue#8019) BUG: Fix bugs for np.ma.count and copy on builtin types (issue #8019) Sep 18, 2016
@ahaldane
Copy link
Member

I'm worried this goes a little too far. What about the case of a subclass of MaskedArray? In the old code the subclass's method would be called, but not anymore here.

Actually, it seems to me that the frommethod code should look extremely similar to the np.all code: It should start by calling something like asanyarray except it should be something like asanyMaskedArray which only converts thjings which are not subclases of maskedarray, and then it should call the resulting objects method. No?

@skwbc
Copy link
Contributor Author

skwbc commented Sep 23, 2016

@ahaldane I didn't care about subclasses of MaskedArray and I fixed the code using np.ma.asanyarray. It corresponds to np.asanyarray for MaskedArray (it conserves subclasses of MaskedArray).

method = getattr(MaskedArray, method_name, None)
if method is not None:
return method(MaskedArray(a), *args, **params)
return method(marr, *args, **params)
Copy link
Member

Choose a reason for hiding this comment

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

It might be nicer not to repeat this line, by flipping the if statement

@@ -6390,18 +6390,14 @@ def __call__(self, a, *args, **params):
arr = args[0]
args[0] = a
a = arr
Copy link
Member

Choose a reason for hiding this comment

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

While we're here, can we change these lines to a, args[0] = args[0], a?

@eric-wieser
Copy link
Member

Otherwise generally looks good to me

@ahaldane
Copy link
Member

ahaldane commented Feb 22, 2017

Oh I must have missed the notification that @skwbc updated (sorry!).

The code looks good to me too.

I would squash all the commits together, and the single commit should say "fixes #8019' in the commit message.

@eric-wieser
Copy link
Member

eric-wieser commented Feb 22, 2017

@ahaldane: I've rebased and squashed this at #8665, since I needed it for something else

@eric-wieser
Copy link
Member

Thanks @skwbc, this was merged in #8665

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