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 into
numpy:masterfrom
skwbc:issue#8019
Closed

skwbc wants to merge 3 commits into
numpy:masterfrom
skwbc:issue#8019

Conversation

@skwbc

@skwbc skwbc commented Sep 18, 2016 •

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

skwbc commented Sep 23, 2016

Copy link
Copy Markdown
Contributor Author

@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).

Comment thread numpy/ma/core.py
method = getattr(MaskedArray, method_name, None)
if method is not None:
return method(MaskedArray(a), *args, **params)
return method(marr, *args, **params)

Copy link
Copy Markdown
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

Comment thread numpy/ma/core.py
@@ -6390,18 +6390,14 @@ def __call__(self, a, *args, **params):
arr = args[0]
args[0] = a
a = arr

Copy link
Copy Markdown
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
Copy Markdown
Member

Otherwise generally looks good to me

@ahaldane

ahaldane commented Feb 22, 2017 •

Copy link
Copy Markdown
Member

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

eric-wieser commented Feb 22, 2017 •

Copy link
Copy Markdown
Member

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

@eric-wieser

Copy link
Copy Markdown
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