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: Fixed previous attempt to fix dimension mismatch in nanpercentile #7180

Merged
merged 1 commit into from
Feb 5, 2016

Conversation

madphysicist
Copy link
Contributor

PR #5981 did not not correctly fix Issue #5760. I have added a test to demonstrate this (which now passes).

Basically, the problem is that np.swapaxes only works if you have two dimensions. The correct function to use is np.rollaxis.

@@ -727,6 +727,9 @@ def test_multiple_percentiles(self):
keepdims=keepdim)
assert_equal(nan_val, val)

megamat = np.ones((3, 4, 5, 6))
assert_equal(np.nanpercentile(megamat, perc, axis=(1, 2)).shape, (2, 3, 6))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Result before change was (2, 6, 3).

@madphysicist
Copy link
Contributor Author

Any status on this one?

@charris
Copy link
Member

charris commented Feb 5, 2016

LGTM, but the commit message needs more explanation of what you are fixing and why. It is almost impossible to write a commit message that is too detailed...

nanpercentile was conforming to dimension convention of percentile incorrectly.
percentile outputs results for the different percentiles along the first
dimension of the output. nanpercentile was moving the reduction axis to the
front using swapaxes, which would move the first axis out of place if there
were more than two in the array. Added a test with more than two axes to
demonstrate and used rollaxis instead of swapaxes to do the interhange.
@madphysicist
Copy link
Contributor Author

Putting your theory to the test :)

@madphysicist
Copy link
Contributor Author

I hope I was clear as well as detailed.

@charris
Copy link
Member

charris commented Feb 5, 2016

Better ;) I wonder if the documentation of percentile is correct in that it refers to the remaining axis as reduced axes whereas I would think they are the unreduced axes.

@madphysicist
Copy link
Contributor Author

I fixed that in #7181. New wording:

If multiple percentiles are given, first axis of the result corresponds to the percentiles. The other axes are the axes that remain after the reduction of a.

@madphysicist
Copy link
Contributor Author

I don't think this break is my fault, unless it is because I need to rebase.

FAIL: test_scripts.test_f2py
...
AssertionError: Warning: neither f2py nor f2py3 nor f2py3.5 found in path

@njsmith
Copy link
Member

njsmith commented Feb 5, 2016

Yeah, that one's not your fault, and a rebase won't help because it's currently failing in master -- see #7197.

@jakirkham
Copy link
Contributor

Probably can rebase on master now and that will fix this failure.

@charris
Copy link
Member

charris commented Feb 5, 2016

Should be able to just restart the failing tests. I'll do that...

@charris
Copy link
Member

charris commented Feb 5, 2016

I'm going to try a close and reopen.

@charris charris closed this Feb 5, 2016
@charris charris reopened this Feb 5, 2016
@jakirkham
Copy link
Contributor

Yeah, AppVeyor doesn't have much love for the close open trick.

@madphysicist
Copy link
Contributor Author

I will rebase in a little bit.

charris added a commit that referenced this pull request Feb 5, 2016
BUG: Fixed previous attempt to fix dimension mismatch in nanpercentile
@charris charris merged commit 48cae08 into numpy:master Feb 5, 2016
@charris
Copy link
Member

charris commented Feb 5, 2016

@madphysicist The merge will get tested.

@madphysicist madphysicist deleted the nanpercentile-dims branch February 6, 2016 02:06
@madphysicist madphysicist restored the nanpercentile-dims branch February 6, 2016 02:17
@charris charris removed this from the 1.11.0 release milestone Feb 7, 2016
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