10000 FIX adjust inliner criterion in RANSACRegressor by gregorystrubel · Pull Request #19499 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

FIX adjust inliner criterion in RANSACRegressor #19499

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 15 commits into from
Jul 30, 2021
Merged

FIX adjust inliner criterion in RANSACRegressor #19499

merged 15 commits into from
Jul 30, 2021

Conversation

gregorystrubel
Copy link
Contributor
@gregorystrubel gregorystrubel commented Feb 19, 2021 •
edited
Loading

Reference Issues/PRs

Fixes #19497.
Fixes #16147.

What does this implement/fix? Explain your changes.

This fix implement solution discussed in #16147:
inlier_mask_subset = residuals_subset <= residual_threshold
that replace the current <

This make the code work also with perfect horizontal lines (see #19497).

Basically I suggest to change the definition of what is an inlier respectivly to residual_threshold. Exactly on the threshold, I suggest to say it is an inlier and not an outliner.

This make a couple of "normal cases" work: horizontal line #19497 (currently we say all the point on a line are outlier!) and other examples described in #16147

Unfortunately This patch make a test fail, but we can keep it with the following modification. The test construct an example where there is no inlier. It is based on the fact when residual_threshold =0 an exception is always thrown. To do this in a clean way we should construct here a real example where there is no inlier. Such an example is not easy to find.
I suggest to construct a more simple test as before : set the residual_threshold to an other corner value that make always the comparison fail : a nan

I also implement the test described in #19497 (horizontal lines)

Any other comments?

Gregory Strubel added 2 commits February 19, 2021 01:25
bugfix RANSAC - residual threshold calculation scikit-learn#16147
fa52a3e
RANSAC test
58e7a5a 
add new test for horizontal line
adapt no inliner test with a synonym
@ogrisel
Copy link
Member
ogrisel commented Feb 23, 2021

Thanks for the fix, the detailed explanation and the new test.

Can you please check if the definition of an inlier needs to be updated in the documentation? If not it might be worth making the doc a bit more specific (either in the user guide or the docstring or both).

Can you also please document your fix in the changelog (doc/what_new/v1.0.rst)?

ogrisel
ogrisel reviewed 8000 Feb 23, 2021
View reviewed changes
@gregorystrubel
Copy link
Contributor Author

Thanks a lot for your review. I have updated the documentation.

@cmarmo cmarmo added this to the 1.0 milestone Mar 2, 2021
Gregory Strubel added 2 commits March 27, 2021 10:41
Merge branch 'main' of https://github.com/scikit-learn/scikit-learn i…
4bef120 
…nto bugfix_ransac_residual_threshold
10000
Merge branch 'main' of https://github.com/scikit-learn/scikit-learn i…
aee83c8 
…nto bugfix_ransac_residual_threshold
jjerphan
jjerphan requested changes Jun 9, 2021
View reviewed changes
Copy link
Member
@jjerphan jjerphan 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 this PR @gregorystrubel! Here are a few suggestions.

gregorystrubel and others added 6 commits June 10, 2021 10:00
@gregorystrubel @jjerphan
Update doc/modules/linear_model.rst
6b933e1 
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
Merge branch 'main' of https://github.com/scikit-learn/scikit-learn i…
d4727e2 
…nto bugfix_ransac_residual_threshold
precise "non-regression" test
ddacfe2 
Relative to jjerphan comment
scikit-learn#19499 (comment)
@gregorystrubel @jjerphan
Update doc/whats_new/v1.0.rst
6cbee9b 
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
Merge branch 'bugfix_ransac_residual_threshold' of github.com:gregory…
66a7abf 
…strubel/scikit-learn into bugfix_ransac_residual_threshold
@gregorystrubel @jjerphan
Update sklearn/linear_model/_ransac.py
93dfad4 
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
@gregorystrubel
Copy link
Contributor Author

Thanks @jjerphan, additionally to your suggestions, I precised "non-regression" in the comment of the non-regression test
(commit #ddacfe2)

@gregorystrubel gregorystrubel requested a review from jjerphan June 10, 2021 15:10
jjerphan
jjerphan approved these changes Jun 10, 2021
View reviewed changes
Copy link
Member
@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @gregorystrubel!

@glemaitre glemaitre self-assigned this Jul 30, 2021
@glemaitre glemaitre changed the title Bugfix ransac residual threshold + horizontal line FIX adjust inliner criterion in RANSACRegressor Jul 30, 2021
glemaitre
glemaitre approved these changes Jul 30, 2021
View reviewed changes
@glemaitre
Copy link
Member

I merged my small suggestions that were only typo. We will check if the tests are passing and we will be able to merge.

@glemaitre glemaitre merged commit 0b45ac5 into scikit-learn:main Jul 30, 2021
@gregorystrubel gregorystrubel deleted the bugfix_ransac_residual_threshold branch August 13, 2021 19:04
samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
@gregorystrubel @jjerphan
@glemaitre @samronsin
FIX adjust inliner criteria in RANSACRegressor (scikit-learn#19499)
5cfecc4 
Co-authored-by: Gregory Strubel <greg@Air-de-Ali.local>
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glemaitre glemaitre glemaitre approved these changes

@jjerphan jjerphan jjerphan approved these changes

@ogrisel ogrisel Awaiting requested review from ogrisel

Assignees

@glemaitre glemaitre

Labels
module:linear_model
Projects
None yet
Milestone
1.0
Development

Successfully merging this pull request may close these issues.

RANSAC - perfect horizontal line generate an exception RANSAC - residual threshold calculation
5 participants
@gregorystrubel @ogrisel @glemaitre @jjerphan @cmarmo
0