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 Add tol to _make_unique to avoid inf values in IsotonicRegression #18639

Merged
merged 12 commits into from Oct 27, 2020

Conversation

lucyleeow
Copy link
Member

@lucyleeow lucyleeow commented Oct 19, 2020

Reference Issues/PRs

Fixes #10903
Closes #10981 and #17758

What does this implement/fix? Explain your changes.

Add a tol when checking equality in _make_unique, such that values are deemed unique only if their difference is greater or equal than 1e-15 i.e.:

if x - current_x >= 1e-15:

this agrees with Python:

>>> 1 == 1+1e-15
False
>>> 1 == 1+1e-16
True

Note that this is not the case for 0:

>>> 0 == 1e-15
False
>>> 0 == 1e-323
False
>>> 0 == 1e-324
True

see more here: #17758 (comment)

Any other comments?

Also removed current_count as it does not seem to be used anymore (as discussed with @ogrisel)
cc @ogrisel

sklearn/_isotonic.pyx Outdated Show resolved Hide resolved
sklearn/_isotonic.pyx Outdated Show resolved Hide resolved
@ogrisel
Copy link
Member

ogrisel commented Oct 21, 2020

Could you please add a new non-regression test for IsotonicRegression based on #17758 (comment) as part of this PR to check that this fixes the source of the original issue?

Copy link
Member

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

Thanks for fixing this nasty bug @lucyleeow! Here are a few more suggestions for improvements. But otherwise LGTM.

sklearn/tests/test_isotonic.py Outdated Show resolved Hide resolved
sklearn/tests/test_isotonic.py Outdated Show resolved Hide resolved
sklearn/_isotonic.pyx Outdated Show resolved Hide resolved
sklearn/tests/test_isotonic.py Outdated Show resolved Hide resolved
sklearn/tests/test_isotonic.py Show resolved Hide resolved
sklearn/tests/test_isotonic.py Outdated Show resolved Hide resolved
@ogrisel
Copy link
Member

ogrisel commented Oct 22, 2020

FYI, I merged the master branch that has a fix for the unrelated failing macOS CI.

@lucyleeow
Copy link
Member Author

Thanks for the suggestions @ogrisel, added!

Copy link
Member

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

This looks good to me! Can you please add a new entry for doc/whats_new/0.24.rst?

@lucyleeow
Copy link
Member Author

Maybe 2nd review @glemaitre 😄 ?

@agramfort
Copy link
Member

thanks @lucyleeow !

@ogrisel ogrisel merged commit ca6d844 into scikit-learn:master Oct 27, 2020
7 checks passed
@lucyleeow lucyleeow deleted the IS/10903 branch October 27, 2020 21:34
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.

CalibratedClassifierCV with mode = 'isotonic' has predict_proba return infinite probabilities
3 participants