Skip to content

[WIP] BUG make _weighted_percentile(data, ones, 50) consistent with numpy.median(data)#17377

Closed
glemaitre wants to merge 52 commits intoscikit-learn:mainfrom
glemaitre:is/17370
Closed

[WIP] BUG make _weighted_percentile(data, ones, 50) consistent with numpy.median(data)#17377
glemaitre wants to merge 52 commits intoscikit-learn:mainfrom
glemaitre:is/17370

Conversation

@glemaitre
Copy link
Copy Markdown
Member

@glemaitre glemaitre commented May 28, 2020 •
edited
Loading

closes #17370
closes #6189

Add a new parameter to select the type of interpolation. By default, the median in NumPy and our implementation should be the same.

glemaitre
glemaitre commented May 28, 2020
View reviewed changes
@adrinjalali
Copy link
Copy Markdown
Member

adrinjalali commented May 29, 2020

I was wondering about this when I was working on sample weights on HGBT. So I'm all in :)

If we have one cython version, then we could use it in other places such as HGBT as well.

lucyleeow
lucyleeow reviewed May 29, 2020
View reviewed changes
lucyleeow
lucyleeow reviewed May 29, 2020
View reviewed changes
@glemaitre
Copy link
Copy Markdown
Member Author

glemaitre commented May 29, 2020

I was wondering about this when I was working on sample weights on HGBT. So I'm all in :)

So I think that I am converging here (the tests with the NumPy are fine). Not sure that we actually need "lower" and "higher". Basically "nearest" should be fast and "linear" would be to match the default of NumPy and allow to interpolate.

If we have one cython version, then we could use it in other places such as HGBT as well.

I really rely on NumPy there. If we are fine internally in the HGBDT to use the nearest I am not sure that we will improve too much by cythonizing.

lucyleeow
lucyleeow reviewed May 29, 2020
View reviewed changes
lucyleeow
lucyleeow reviewed May 29, 2020
View reviewed changes
@lucyleeow lucyleeow mentioned this pull request Jun 17, 2020
Merged
glemaitre
glemaitre commented Jun 22, 2020
View reviewed changes
sklearn/ensemble/tests/test_gradient_boosting.py
if last_y_pred is not None:
assert_array_almost_equal(last_y_pred, y_pred)

assert_allclose(last_y_pred, y_pred)
Copy link
Copy Markdown
Member Author

@glemaitre glemaitre Jun 22, 2020

Choose a reason for hiding this comment

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

@lucyleeow I added your test here. Using the strategy nearest everywhere seems to be the winner here to be able to keep the sample_weight semantic rights.

@glemaitre
Copy link
Copy Markdown
Member Author

glemaitre commented Jun 22, 2020

I merged a couple of things in this PR to be able to check the integration in the gradient boosting as well.

If the test are passing I would propose to cut this PR into 3 parts:

  • implementation of the _weighted_percentile with its own test;
  • changing boston with make_regression in the gradient boosting;
  • change the call from np.median to np.percentile(..., interpolation="nearest"to ensure that we have the same behaviour withsample_weight=None and sample_weight=np.ones(...).

@jnothman
Copy link
Copy Markdown
Member

jnothman commented Jun 25, 2020

If the test are passing I would propose to cut this PR into 3 parts:

So this is no longer for review?

@glemaitre
Copy link
Copy Markdown
Member Author

glemaitre commented Jun 29, 2020

No, I think that I need to cut it into smaller pieces because the diff is too large. I don't want to make the reviewing process even harder. I will take care of it this week.

@glemaitre glemaitre mentioned this pull request Jun 29, 2020
Closed
@glemaitre glemaitre changed the title BUG make _weighted_percentile behave as NumPy [WIP] BUG make _weighted_percentile behave as NumPy Jun 29, 2020
@glemaitre glemaitre mentioned this pull request Jun 29, 2020
Closed
Base automatically changed from master to main January 22, 2021 10:52
@ogrisel ogrisel mentioned this pull request Jan 5, 2022
Closed
@ogrisel ogrisel changed the title [WIP] BUG make _weighted_percentile behave as NumPy [WIP] BUG make _weighted_percentile(data, ones, 50) consistent with numpy.median(data) Jan 16, 2024
@lorentzenchr
Copy link
Copy Markdown
Member

lorentzenchr commented Apr 26, 2024

See #17370 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ogrisel ogrisel ogrisel left review comments

@lucyleeow lucyleeow lucyleeow left review comments

Assignees

No one assigned

Labels

module:utils

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

_weighted_percentile does not lead to the same result than np.median Make _weighted_percentile more robust

6 participants

@glemaitre @adrinjalali @lucyleeow @ogrisel @jnothman @lorentzenchr