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: weighted nanpercentile, nanquantile and multi-dim q #26582

Merged
merged 6 commits into from
Jun 14, 2024

Conversation

lorentzenchr
Copy link
Contributor

Fixes #26455.

Hint: The solution is no beauty, but it does seem to do it's job. The whole nanpercentile seems convoluted given that it could rely more on percentile.

@ngoldbaum ngoldbaum added this to the 2.0.0 release milestone May 31, 2024
@ngoldbaum ngoldbaum changed the title FIX: weighted nanpercentile and nanquantile BUG: weighted nanpercentile and nanquantile May 31, 2024
@ngoldbaum
Copy link
Member

Linter failures are real. I unfortunately don't have time to understand the code to give you a proper code review right now.

@lorentzenchr
Copy link
Contributor Author

lorentzenchr commented Jun 3, 2024

@seberg might be interested in this one. I try to fix the bugs I create.
@ogrisel detected this bug. Tests are taken from your issue.

@rgommers rgommers requested a review from seberg June 12, 2024 18:19
@rgommers rgommers added the 09 - Backport-Candidate PRs tagged should be backported label Jun 12, 2024
Copy link
Contributor

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

I don't understand all the black magic tricks of the slicing/ndindexing expressions but the test looks good.

numpy/lib/tests/test_nanfunctions.py Outdated Show resolved Hide resolved
@seberg
Copy link
Member

seberg commented Jun 14, 2024

I pushed some changes, to simplify the logic with moveaxes and add tests for multi-dimensional inputs.

The problem if you add tests... is that you find the other bugs in the old code paths and need to fix them too. Multi-dimensional q wasn't properly implemented...

Copy link
Member

@seberg seberg left a comment

Choose a reason for hiding this comment

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

But someone else should have a quick look over...

@seberg seberg changed the title BUG: weighted nanpercentile and nanquantile BUG: weighted nanpercentile, nanquantile and multi-dim q Jun 14, 2024
@seberg seberg changed the title BUG: weighted nanpercentile, nanquantile and multi-dim q BUG: weighted nanpercentile, nanquantile and multi-dim q Jun 14, 2024
@charris charris merged commit 81763d5 into numpy:main Jun 14, 2024
65 of 68 checks passed
@charris
Copy link
Member

charris commented Jun 14, 2024

Thanks @lorentzenchr and reviewers.

charris pushed a commit to charris/numpy that referenced this pull request Jun 15, 2024
* TST add test_nan_value_with_weight

* FIX weighted nanquantile

* CLN appease linter

* Update numpy/lib/tests/test_nanfunctions.py

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

* Simplify code, expand old and add new ndim test with weights

* BUG: Expand test and fix multi-dimensional q in the normal path...

---------

Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Sebastian Berg <sebastianb@nvidia.com>
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Jun 15, 2024
@lorentzenchr lorentzenchr deleted the nanpercentile branch June 17, 2024 06:59
ArvidJB pushed a commit to ArvidJB/numpy that referenced this pull request Nov 1, 2024
* TST add test_nan_value_with_weight

* FIX weighted nanquantile

* CLN appease linter

* Update numpy/lib/tests/test_nanfunctions.py

Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

* Simplify code, expand old and add new ndim test with weights

* BUG: Expand test and fix multi-dimensional q in the normal path...

---------

Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Sebastian Berg <sebastianb@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected behavior of nanpercentile with weighted missing values
6 participants