Skip to content
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

BUG: np.ma.mean and var should return scalar if no mask #8142

Merged
merged 1 commit into from
Oct 14, 2016

Conversation

ahaldane
Copy link
Member

Fixes #5769

This is a followup to #7350 (first added in 1.12), and makes sure np.ma.mean and np.ma.var return a scalar if appropriate in the nomask branch of the code.

@ahaldane
Copy link
Member Author

Hmm this led me to notice some other (old) problems with np.ma.var I might fix here at the same time. For example, currently (without this PR) var returns ndarrays instead of maskedarrays if mask is nomask. Don't merge yet.

cnt = self.count(axis=axis, **kwargs) - ddof
danom = self - self.mean(axis, dtype, keepdims=True)
if iscomplexobj(self):
danom = umath.absolute(danom) ** 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Good to check for a complex object, but might as well do (there was a PR to make a ufunc out of this...):

danom = danom.real**2 + danom.imag**2

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to leave this alone for now, as outside the scope of this PR. (this code is already present).

@charris
Copy link
Member

charris commented Oct 13, 2016

@ahaldane Still planning more for this PR?

@ahaldane
Copy link
Member Author

Yeah, I need a day or two more to decide what to do in var.

I have an old branch lying around with a "proper" overhaul of np.ma.mean and np.ma.var, but I think for this PR I only want to make as minimal changes to var as possible while still fixing #5769.

@ahaldane
Copy link
Member Author

I finished my changes here, it should be ready to go.

@@ -5057,7 +5057,7 @@ def mean(self, axis=None, dtype=None, out=None, keepdims=np._NoValue):

if self._mask is nomask:
result = super(MaskedArray, self).mean(axis=axis,
dtype=dtype, **kwargs)
dtype=dtype, **kwargs)[()]
else:
Copy link
Member

Choose a reason for hiding this comment

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

I assume the super mean returns a 0-d array scalar for relevant cases. I note the following odd behavior of ordinary arrays that is new to me.

>>> np.float64(1)
1.0
>>> np.float64(1)[()]
array(1.0)
>>> np.float64(1)[()][()]
1.0

That is, the [()] construct flips the result back and forth between arrays and scalars.

Copy link
Member

@eric-wieser eric-wieser Feb 28, 2017

Choose a reason for hiding this comment

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

For anyone coming here in future, the above is no longer (1.11.1) the case. [()] does not promote scalars to 0d-arrays

@charris
Copy link
Member

charris commented Oct 14, 2016

I note some odd behavior of [()] indexing that has me a bit worried. For instance with ordinary mean

>>> np.mean([1,2,3])[()]
array(2.0)

@ahaldane
Copy link
Member Author

What numpy version? I don't get that on 1.11, and I think there is a lot of numpy code depends on [()] returning a scalar.

@ahaldane
Copy link
Member Author

Hmm, but actually maybe you are right. I just remembered #7267.

And actually grepping shows the empty tumple indexing is only used in a few places in python, and I know of only one place in C it is used.

@charris
Copy link
Member

charris commented Oct 14, 2016

I'm running on my travel machine that has numpy 1.8. I can't upgrade until I figure out how to get Cython installed. Yeah, my mac environment is a mess, I don't use it often enough...

@charris
Copy link
Member

charris commented Oct 14, 2016

I'm not particularly worried as long as the tests cover the relevant possibilities.

@charris charris merged commit fa31422 into numpy:master Oct 14, 2016
@charris
Copy link
Member

charris commented Oct 14, 2016

If you can't replicate what I'm seeing on 1.8 let's give it a shot. Thanks Allan.

@ahaldane
Copy link
Member Author

Thanks Chuck!

charris pushed a commit to charris/numpy that referenced this pull request Oct 17, 2016
As discussed in my comments for issue numpy#8145, this patch adds the
equal_nan argument to assert_array_compare(), and assert_allclose()
passes the value it receives for the same argument through to
assert_array_compare().

Closes numpy#8142.
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.

None yet

4 participants