FIX adjust inliner criterion in RANSACRegressor #19499
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
Feb 19, 2021
add new test for horizontal line adapt no inliner test with a synonym
|
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 ( |
ogrisel
reviewed
Feb 23, 2021
added 2 commits
Feb 27, 2021
…nto bugfix_ransac_residual_threshold
|
Thanks a lot for your review. I have updated the documentation. |
added 2 commits
Mar 27, 2021
…nto bugfix_ransac_residual_threshold
…nto bugfix_ransac_residual_threshold
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
…nto bugfix_ransac_residual_threshold
Relative to jjerphan comment scikit-learn#19499 (comment)
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
…strubel/scikit-learn into bugfix_ransac_residual_threshold
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
|
Thanks @jjerphan, additionally to your suggestions, I precised "non-regression" in the comment of the non-regression test |
glemaitre
changed the title
glemaitre
approved these changes
Jul 30, 2021
|
I merged my small suggestions that were only typo. We will check if the tests are passing and we will be able to merge. |
samronsin
added a commit
to samronsin/scikit-learn
that referenced
this issue
Nov 30, 2021
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
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?
The text was updated successfully, but these errors were encountered: