Skip to content

BUG: Handle multiple percentiles for all-nan slices in nanpercentile #5981

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 17, 2016
Merged

BUG: Handle multiple percentiles for all-nan slices in nanpercentile #5981

merged 1 commit into from
Jan 17, 2016

Conversation

dfreese
Copy link

@dfreese dfreese commented Jun 18, 2015

Fixes #5760

@dfreese dfreese force-pushed the fix/multi_percent_nanperc_bug branch from 776e550 to ade92c2 Compare June 18, 2015 19:47
@shoyer
Copy link
Member

shoyer commented Jun 23, 2015

This needs tests to verify that the behavior is fixed.

@dfreese dfreese force-pushed the fix/multi_percent_nanperc_bug branch from ade92c2 to 4fdc1a4 Compare June 29, 2015 18:27
@dfreese
Copy link
Author

dfreese commented Jun 29, 2015

Turns out the behavior of nanpercentile with multiple percentiles was not replicating percentile. This has been fixed, and tests were added to ensure the behavior matches. This clears up the the all-nan slices case, which is also checked in the tests that were added.

@ssanderson
Copy link

@dfreese it looks like there's also some odd behavior in the current release relating to the keepdims flag. Consider:

In [1]: data = eye(5, dtype=float64)

In [2]: percentile(data, [60, 80], keepdims=True, axis=1)
Out[2]:
array([[[ 0. ],
        [ 0. ],
        [ 0. ],
        [ 0. ],
        [ 0. ]],

       [[ 0.2],
        [ 0.2],
        [ 0.2],
        [ 0.2],
        [ 0.2]]])

In [3]: nanpercentile(data, [60, 80], keepdims=True, axis=1)
Out[3]:
array([[[ 0. ],
        [ 0.2],
        [ 0. ],
        [ 0.2],
        [ 0. ]],

       [[ 0.2],
        [ 0. ],
        [ 0.2],
        [ 0. ],
        [ 0.2]]])
In [6]: np.version.version
Out[6]: '1.9.2'

Any idea if your changes here fix these this issue as well?

seberg
seberg reviewed Jun 29, 2015
View reviewed changes
numpy/lib/nanfunctions.py
if q.ndim == 0:
return np.nan
else:
return np.nan * np.ones((len(q),))
Copy link
Member

Choose a reason for hiding this comment

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

Just to make sure, but q may only be 0-D or 1-D at this point?! Could you maybe add a test for the higher dimensional case.

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe we do just don't support the higher dim case for nanpercentile.

Copy link
Author

Choose a reason for hiding this comment

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

I didn't consider that because it doesn't appear that percentile supports the higher dimensional case either. The following results in a ValueError being raised.

import numpy as np
data = np.eye(5, dtype=np.float64)
np.percentile(data, [[60, 80], [90, 100]], keepdims=True, axis=1)

Copy link
Member

Choose a reason for hiding this comment

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

Right, thought we had added that, but must have been some other thing (maybe it was just how q is interpreted if 1-d).

@dfreese
Copy link
Author

dfreese commented Jun 29, 2015

@ssanderson, just checked and this would take care of that issue, though the tests could be updated to be more explict about that.

In [1]: import numpy as np

In [2]: data = np.eye(5, dtype=np.float64)

In [3]: np.percentile(data, [60, 80], keepdims=True, axis=1)
Out[3]: 
array([[[ 0. ],
        [ 0. ],
        [ 0. ],
        [ 0. ],
        [ 0. ]],

       [[ 0.2],
        [ 0.2],
        [ 0.2],
        [ 0.2],
        [ 0.2]]])

In [4]: np.nanpercentile(data, [60, 80], keepdims=True, axis=1)
Out[4]: 
array([[[ 0. ],
        [ 0. ],
        [ 0. ],
        [ 0. ],
        [ 0. ]],

       [[ 0.2],
        [ 0.2],
        [ 0.2],
        [ 0.2],
        [ 0.2]]])

@dfreese dfreese force-pushed the fix/multi_percent_nanperc_bug branch from 4fdc1a4 to 65708e7 Compare June 30, 2015 00:21
@dfreese
Copy link
Author

dfreese commented Jun 30, 2015

Tests have been updated to be more explicit about the issues pointed out by @ssanderson.

@dfreese dfreese closed this Jun 30, 2015
@dfreese dfreese reopened this Jun 30, 2015
@dfreese
Copy link
Author

dfreese commented Jun 30, 2015

Closing and reopening, because Travis CI failed on an out of disk space error. Trying the tests again.

@ssanderson
Copy link

@dfreese this looks like it's passing now?

@dfreese
Copy link
Author

dfreese commented Jul 2, 2015

@ssanderson, Yeah, looks good from my end, though I'm open to additional comments.

@dfreese dfreese force-pushed the fix/multi_percent_nanperc_bug branch from 65708e7 to 2bc6a4c Compare July 25, 2015 06:19
@dfreese
Copy link
Author

dfreese commented Jul 25, 2015

rebase to upstream master.

@dfreese dfreese force-pushed the fix/multi_percent_nanperc_bug branch from 2bc6a4c to 227c79d Compare August 24, 2015 05:06
@dfreese
Copy link
Author

dfreese commented Aug 24, 2015

rebased again.

@dfreese dfreese force-pushed the fix/multi_percent_nanperc_bug branch from 227c79d to c1fd0ad Compare September 1, 2015 22:44
@dfreese
Copy link
Author

dfreese commented Sep 1, 2015

@charris just saw that 1.10 is in RC. Will this sort of fix need to wait until 1.11?

@dfreese dfreese force-pushed the fix/multi_percent_nanperc_bug branch from c1fd0ad to d7bc812 Compare January 12, 2016 21:21
@dfreese
Copy link
Author

dfreese commented Jan 12, 2016

Rebased again. Can this be merged?

charris
charris reviewed Jan 12, 2016
View reviewed changes
numpy/lib/tests/test_nanfunctions.py
warnings.simplefilter('always')
val = np.percentile(mat, perc, axis=axis, keepdims=keepdim)
nan_val = np.nanpercentile(nan_mat, perc, axis=axis,
keepdims=keepdim)
Copy link
Member

Choose a reason for hiding this comment

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

PEP8, line up under nan_mat.

Copy link
Member

Choose a reason for hiding this comment

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

Also below.

@charris
Copy link
Member

charris commented Jan 12, 2016

Couple of style nits. Could you also expand the commit message to describe what you have done? Fixes #5760 is a bit terse. @seberg @juliantaylor Comments.

BUG: Fix nanpercentile crash on all-nan slices
6e69994 
Fix bug where nanpercentile would crash with an all-nan slices when
given multiple percentiles.  Also corrects behavior where array sizes
different from numpy.percentile would be returned with keepdims enabled.

Fix numpy#5760
@dfreese dfreese force-pushed the fix/multi_percent_nanperc_bug branch from d7bc812 to 6e69994 Compare January 12, 2016 22:46
@dfreese
Copy link
Author

dfreese commented Jan 12, 2016

style was fixed up, and commit message updated.

@juliantaylor
Copy link
Contributor

looks good, thanks

juliantaylor added a commit that referenced this pull request Jan 17, 2016
@juliantaylor
Merge pull request #5981 from dfreese/fix/multi_percent_nanperc_bug
28fdcf3 
BUG: Handle multiple percentiles for all-nan slices in nanpercentile
@juliantaylor juliantaylor merged commit 28fdcf3 into numpy:master Jan 17, 2016
@madphysicist madphysicist mentioned this pull request Feb 3, 2016
Merged
@charris charris modified the milestone: 1.11.0 release Feb 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
01 - Enhancement component: numpy._core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dfreese @shoyer @ssanderson @charris @juliantaylor @seberg