8000 BUG: ma.median alternate fix for #7592 by AmitAronovitch · Pull Request #7635 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

Comments

BUG: ma.median alternate fix for #7592#7635

Merged
ahaldane merged 2 commits intonumpy:masterfrom
AmitAronovitch:ma_median_fix
May 22, 2016
Merged

BUG: ma.median alternate fix for #7592#7635
ahaldane merged 2 commits intonumpy:masterfrom
AmitAronovitch:ma_median_fix

Conversation

@AmitAronovitch
Copy link
Contributor
@AmitAronovitch AmitAronovitch commented May 14, 2016

Please see discussion in #7592
For the 1d case: use the original solution (copied from original code, before merge of #4760)
For the n-dim case: leave implementation as before.

@AmitAronovitch AmitAronovitch mentioned this pull request May 14, 2016
Closed
@charris charris changed the title ma.median: alternate fix for #7592 BUG: ma.median alternate fix for #7592 May 14, 2016
@charris charris added this to the 1.11.1 release milestone May 14, 2016
charris
charris reviewed May 14, 2016
View reviewed changes
numpy/ma/extras.py Outdated
Copy link
Member
@charris charris May 14, 2016

Choose a reason for hiding this comment

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

Spaces around ==.

@charris
Copy link
Member
charris commented May 14, 2016

@ahaldane Ping.

@ahaldane
Copy link
Member
ahaldane commented May 15, 2016

Either solution (this or #7592) seems fine to me, at least with the suggested changes in either PR. On the plus side this PR avoids the convoluted isinstance checks, on the downside it introduces a separate branch to the code.

Let's not be like "Burdian's Ass" who can't choose a pile of hay!
This PR seems slighly less tricky, how about going with this one?

I think to tidy it up you should follow @charris' sugestions. I also think it might be better not to have a separate _median1D method, and instead simply move its code to be inside the if-block in _median, since with @charris's suggestion it is only 6 lines long.

I'd also suggest you change low = asorted[ind] to low = asorted[tuple(ind)] and same for high. Even though it won't change any of the results it reduces the chance of miscellaneous bugs down the line

@AmitAronovitch
Copy link
Contributor Author
AmitAronovitch commented May 19, 2016

(see also my comment above in @charris's thread)

@ahaldane
Copy link
Member
ahaldane commented May 20, 2016

Looks good to me! I would like to merge.

@AmitAronovitch one or two final things: I would get rid of the parenthesis in if (asorted.ndim == 1) (it's more pythonic without them), and maybe also change mean(0) to just mean() if that's correct.

Further, I plan to squash everything together if you don't do it for me. If you do end up squashing, could you format the commit message according to the guidelines? In the commit message you should also include the sentence "Fixes #5969." so that git auto-closes it for us.

@AmitAronovitch
BUG: ma.median of 1d array should return a scalar
bb46a49 
Fixes numpy#5969.
Performance fix numpy#4760 had caused wrong shaped results in the 1D case.
This fix restores the original 1D behavior.
@AmitAronovitch
STY: ma.extras.median: avoid indexing with list
a4cc361 
Recommended type for nd-indexing is a tuple. See numpy#4434
@AmitAronovitch
Copy link
Contributor Author
AmitAronovitch commented May 21, 2016

I kept the casting to tuple out of the squashed commit, since it is independent of the bugfix.
Hope that's OK

@ahaldane
Copy link
Member
ahaldane commented May 22, 2016

Looks good, thanks a lot for all the revisions @AmitAronovitch!

Merging now.

@ahaldane ahaldane merged commit 6ce33a1 into numpy:master May 22, 2016
@charris charris mentioned this pull request May 22, 2016
Merged
@charris charris removed this from the 1.11.1 release milestone May 22, 2016
@larrybradley larrybradley mentioned this pull request Jul 14, 2016
Closed
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

06 - Regression component: numpy.ma masked arrays

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@AmitAronovitch @charris @ahaldane
0