Skip to content

BUG: Fixed previous attempt to fix dimension mismatch in nanpercentile - #7180

Merged
charris merged 1 commit into
numpy:masterfrom
madphysicist:nanpercentile-dims
Feb 5, 2016
Merged

BUG: Fixed previous attempt to fix dimension mismatch in nanpercentile#7180
charris merged 1 commit into
numpy:masterfrom
madphysicist:nanpercentile-dims

Conversation

@madphysicist

@madphysicist madphysicist commented Feb 3, 2016

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

madphysicist
madphysicist reviewed Feb 3, 2016
View reviewed changes
Comment thread numpy/lib/tests/test_nanfunctions.py

@madphysicist madphysicist Feb 3, 2016

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

@charris charris added this to the 1.11.0 release milestone Feb 3, 2016
@madphysicist madphysicist mentioned this pull request Feb 4, 2016
Merged
@madphysicist

madphysicist commented Feb 5, 2016

Copy link
Copy Markdown
Contributor Author

Any status on this one?

@charris

charris commented Feb 5, 2016

Copy link
Copy Markdown
Member

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

@madphysicist
BUG: Fixed previous attempt to fix dimension mismatch in nanpercentile
e30d802 
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

madphysicist commented Feb 5, 2016

Copy link
Copy Markdown
Contributor Author

Putting your theory to the test :)

@madphysicist

madphysicist commented Feb 5, 2016

Copy link
Copy Markdown
Contributor Author

I hope I was clear as well as detailed.

@charris

charris commented Feb 5, 2016

Copy link
Copy Markdown
Member

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

madphysicist commented Feb 5, 2016

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

madphysicist commented Feb 5, 2016

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

njsmith commented Feb 5, 2016

Copy link
Copy Markdown
Member

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

@jakirkham

jakirkham commented Feb 5, 2016

Copy link
Copy Markdown
Contributor

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

@charris

charris commented Feb 5, 2016

Copy link
Copy Markdown
Member

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

@charris

charris commented Feb 5, 2016

Copy link
Copy Markdown
Member

I'm going to try a close and reopen.

@charris charris closed this Feb 5, 2016
@charris charris reopened this Feb 5, 2016
@jakirkham

jakirkham commented Feb 5, 2016

Copy link
Copy Markdown
Contributor

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

@madphysicist

madphysicist commented Feb 5, 2016

Copy link
Copy Markdown
Contributor Author

I will rebase in a little bit.

charris added a commit that referenced this pull request Feb 5, 2016
@charris
Merge pull request #7180 from madphysicist/nanpercentile-dims
48cae08 
BUG: Fixed previous attempt to fix dimension mismatch in nanpercentile
@charris
charris merged commit 48cae08 into numpy:master Feb 5, 2016
@charris

charris commented Feb 5, 2016

Copy link
Copy Markdown
Member

@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
@charris charris mentioned this pull request Feb 7, 2016
Merged
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

00 - Bug component: numpy.lib

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@madphysicist @charris @njsmith @jakirkham