BUG: Fixed ``where`` keyword for ``np.mean`` & ``np.var`` methods by mtsokol · Pull Request #18560 · numpy/numpy · GitHub

Addresses #18552

I've managed to fix where keyword issue by refactoring those if statements.

So the rcount is a scalar when where is not provided as we reduce equal number of elements for given axis (or axes). So the first and statement prevents from illegal comparison of array to 0 value that caused original error. If the where clause is provided and rcount is an array (as we might want to reduce different number of elements along axis due to masking) then we check second and statement if any of those reduce groups are zero. I also maintained this micro-optimization to avoid calling _any(rcount == 0) when where is True and not zero.

Is this correct?

Also I had to change umr_any to _any as I think there is another bug there (or umr_any shouldn't be used there). np.any without axis parameter evaluates to scalar value (that can be used in if statement). But umr_any does not behave that way - it only reduces one dimension. Here's code for reproducing:

import numpy as np
from numpy.core import umath as um
a = np.ones((3,4))

# used in numpy/core/_methods.py
umr_any = um.logical_or.reduce

# np.any works as expected
if np.any(a == 0):
    pass

# but umr_any can't be used in if statement as here in `_mean`
if umr_any(a == 0):
    pass

Here's a colab reproduction: https://colab.research.google.com/drive/1UJi6E50zzaLu2nZQWMEFHPLZnq2v6pzT?usp=sharing

Thank you for any help!

So, after puzzling longer... The error I made is the missing axis=None here. It seems that unlike our top-level aliases, ufunc.reduce uses axis=0 by default. And I frankly would not have known that without stumbling over it even otherwise...

It was silly to delete it, but just as bad that none of these have tests that fail here. Could you use the axis=None solution (could pass by position if you like), and add a simple test?

@seberg Sure! I've just pushed changes reverting to umr_any and adding test case

I don't get this logic - surely _count_reduce_items can return an nd array even if where is True, and this will still break?

I think it's impossible for _count_reduce_items to return ndarray with where=True. In such case it returns a scalar items = nt.intp(1) with accumulated items items *= .... Have you got an example where it returns an array when where=True?

Ah you're right, it doesn't do what I think it does.

Thanks for adding a test. Is it just me? I understand that the ternary operator was a bit confusing trying to understand whats wrong, but I can read it much easier than the duplicated where is True check, personally?

Yeah, it was confusing for me 😅. Should I revert it to original if ... if .. else:?

Maybe make a temporary variable with a nice name if you like to avoid the two if on a single line?

On the second thought I revered it to original notation as it's all clear to me now, sorry for this confusion. All pushed.

I think it's ready but I see that one of the CI stages failed that previously was passing (previously whole CI was green). Also I see that it doesn't regard code itself. Should I do something about it?

Thanks! I did a force push just to retrigger CI after PR rename - it's all green now.

Sorry, looks good to me, and happy to merge. If you are still up for it, a test for both std and var would be nice, if not, also fine.

Sorry, looks good to me, and happy to merge. If you are still up for it, a test for both std and var would be nice, if not, also fine.

Sure! Give me 20 minutes. Can I just copy-paste that part with correct _res to std and var?

Thanks @mtsokol lets put this in! :).

Great!
If there's a next not-so-hard issue to take please let me know! If there isn't right now I will check issues page somewhere in the future.

…mpygh-18560) * Fixed keyword bug * Added test case * Reverted to original notation * Added tests for var and std Closes numpygh-18552

Fixed keyword bug

afe2ef1

github-actions Bot added the 00 - Bug label Mar 6, 2021

mtsokol mentioned this pull request Mar 6, 2021

Added 'where' keyword to 'jnp.{mean, var, std}' jax-ml/jax#5966

Merged

seberg reviewed Mar 8, 2021

View reviewed changes

Added test case

9450420

eric-wieser reviewed Mar 9, 2021

View reviewed changes

eric-wieser approved these changes Mar 9, 2021

View reviewed changes

seberg added the 09 - Backport-Candidate PRs tagged should be backported label Mar 9, 2021

charris changed the title BUG: Fixed where keyword for np.mean & np.var methods BUG: Fixed where keyword for np.mean & np.var methods Mar 10, 2021

Reverted to original notation

6e3dc4e

Added tests for var and std

cf75309

seberg merged commit c5de5b5 into numpy:main Mar 11, 2021

seberg mentioned this pull request Mar 11, 2021

numpy.{mean, var, std} throw ValueError when axis and where are specified #18491

Closed

charris pushed a commit to charris/numpy that referenced this pull request Mar 14, 2021

BUG: Fixed where keyword for np.mean & np.var methods (nu…

f7c52ab

…mpygh-18560) * Fixed keyword bug * Added test case * Reverted to original notation * Added tests for var and std Closes numpygh-18552

charris mentioned this pull request Mar 14, 2021

BUG: Fixed where keyword for np.mean & np.var methods #18612

Merged

charris removed the 09 - Backport-Candidate PRs tagged should be backported label Mar 14, 2021

ngoldbaum added Numpy 2.0 API Changes and removed Numpy 2.0 API Changes labels Aug 29, 2023

                   rcount = _count_reduce_items(arr, axis, keepdims=keepdims, where=where)
-                  if rcount == 0 if where is True else umr_any(rcount == 0):
+                  if (where is True and rcount == 0) or (where is not True and _any(rcount == 0)):

BUG: Fixed ``where`` keyword for ``np.mean`` & ``np.var`` methods by mtsokol · Pull Request #18560 · numpy/numpy · GitHub

Uh oh!

BUG: Fixed where keyword for np.mean & np.var methods - #18560

BUG: Fixed where keyword for np.mean & np.var methods#18560seberg merged 4 commits intonumpy:mainnumpy/numpy:mainfrom mtsokol:np-mean-where-keyword-bugCopy head branch name to clipboard

Conversation

mtsokol commented Mar 6, 2021

Uh oh!

seberg Mar 8, 2021

Choose a reason for hiding this comment

Uh oh!

mtsokol Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

eric-wieser Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

mtsokol Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

seberg Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

mtsokol Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

seberg Mar 9, 2021

Choose a reason for hiding this comment

Uh oh!

mtsokol Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtsokol commented Mar 10, 2021

Uh oh!

mtsokol commented Mar 11, 2021

Uh oh!

seberg commented Mar 11, 2021

Uh oh!

mtsokol commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seberg commented Mar 11, 2021

Uh oh!

mtsokol commented Mar 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

BUG: Fixed where keyword for np.mean & np.var methods#18560
seberg merged 4 commits into
numpy:mainfrom
mtsokol:np-mean-where-keyword-bug

mtsokol Mar 9, 2021 •
edited
Loading

mtsokol Mar 9, 2021 •
edited
Loading

mtsokol commented Mar 11, 2021 •
edited
Loading