Repository: scikit-learn/scikit-learn
Pull request: API deprecate default of `min_samples` for `RANSACRegressor`
<!--
Thanks for contributing a pull request! Please ensure you have taken a look at
the contribution guidelines: https://github.com/scikit-learn/scikit-learn/blob/main/CONTRIBUTING.md
-->

#### Reference Issues/PRs
<!--
Example: Fixes #1234. See also #3456.
Please use keywords (e.g., Fixes) to create link to the issues or pull requests
you resolved, so that they will automatically be closed when your pull request
is merged. See https://github.com/blog/1506-closing-issues-via-pull-requests
-->
Closes #6437
Takes over stalled PR and closes #7716


#### What does this implement/fix? Explain your changes.
By default, RANSACRegressor uses a sample size based on the number of features in the input, which is appropriate for linear regression (which is used by default) but not for other models. This patch raises a ValueError if a model is used other than linear regression and no sample size is provided.


#### Any other comments?


<!--
Please be aware that we are a loose team of volunteers so patience is
necessary; assistance handling other issues is very welcome. We value
all user contributions, no matter how minor they are. If we are slow to
review, either the pull request needs some benchmarking, tinkering,
convincing, etc. or more likely the reviewers are simply busy. In either
case, we ask for your understanding during the review process.
For more information, see our FAQ on this topic:
http://scikit-learn.org/dev/faq.html#why-is-my-pull-request-not-getting-any-attention.

Thanks for contributing!
-->

There are many failing checks from test_estimators due to the raised ValueError - if anyone can point me to a suitable place to update the checks I would be very grateful

Problem statement:
Better default min_samples for RANSACRegressor?
`min_samples` is set by default to be `n_features+1`

This fails when `n_features + 1` > `n_samples`, since at every iteration `min_samples` iteration is required.

I don't really know what a better default would be for this case would be

Hints from issue discussion:
cc @ahojnnes

@MechCoder I would raise a ValueError in this case with a meaningful message.

> @MechCoder I would raise a ValueError in this case with a meaningful message.

I think that we need a better default, because the user would be getting
an error message on a parameter that he did not set.

What is the rationale behind setting default `min_samples` to be `n_features + 1`. To make that sure that each sampling forms a determined system in case `LinearRegression` is used? Or is it something else? (I could not figure it out when I had a quick glance at the paper)

@MechCoder I did not implement that default. The only case where it makes sense is the LinearRegression as far as I know.

We could at least do something like `min(n_features + 1, n_samples)` but probably want to look at the literature more closely?

I'm interested in doing this.

I think think that taking `min(n_features + 1, n_samples)` will not work because RANSAC needs a sub-sample of data. `min(n_features + 1, n_samples - 1)` would be better but from my understanding of RANSAC it's important that the sample size is relatively small compared to the number of examples so that it's possible to get a sample with no outliers. Also, I don't know of any regression where it is reasonable to have fewer samples than dimensions. So raising an error rather than silently giving bad results may be the least worst option.

After taking a look, the current behavior is already to raise ValueError with a reasonable message.

We could do `min(n_features + 1, 0.1 * n_samples)`? We could look at the ransac literature to see if that would be appropriate? I feel if `n_features ~ n_samples` ransac is probably a bad choice anyhow, though. Maybe add to the ValueError that it's a bad idea @GaelVaroquaux ?

From the wikipedia page I gather that "n_features + 1" is chosen as the "minimum number of samples needed to fit a linear model".

I've done a little literature reading, and the correct sample size seems to strongly depend on the estimator. Also, the algorithm is known to be finicky and require tuning the hyperparameters to get good results.

The most common "real" application for this algorithm is computer vision applications where it's used to find homography, i.e. a mapping of pixels between stereo images. In this case, the number of features (which are pixels from a slice of an image) is big, like 64 or 100, and the sample size used is always 4. For other kinds of image transformations, the sample size is smaller (for example, for rigid motion with a stationary camera you can use 2). In these cases, using `min(n_features + 1, 0.1 * n_samples)` won't work because the correct sample size doesn't actually depend on the number of samples.

So I guess it doesn't make sense to warn the user if `n_features >= n_samples` since it's entirely appropriate in some situations.

Perhaps a solution would be to check if `base_estimator` is `LinearRegression` and in that case use `n_features + 1`, otherwise use 4? And then if the user is doing something else they're going to have to do some work to find correct hyperparameters anyway because none of the defaults probably work.

Commit message: Update sklearn/linear_model/_ransac.py

Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>

Patch to review:
diff --git a/doc/whats_new/v1.0.rst b/doc/whats_new/v1.0.rst
index 3b23360fe60c4..6b407ce97e0f7 100644
--- a/doc/whats_new/v1.0.rst
+++ b/doc/whats_new/v1.0.rst
@@ -417,6 +417,11 @@ Changelog
 - |Enhancement| Validate user-supplied gram matrix passed to linear models
   via the `precompute` argument. :pr:`19004` by :user:`Adam Midvidy <amidvidy>`.
 
+- |API| Raise a warning in :class:`linear_model.RANSACRegressor` that from
+  version 1.2, `max_samples` need to be set explicitly for model other than
+  :class:`linear_model.LinearRegression`.
+  :pr:`19390` by :user:`Shao Yang Hong <hongshaoyang>`.
+
 - |Fix| :meth:`ElasticNet.fit` no longer modifies `sample_weight` in place.
   :pr:`19055` by `Thomas Fan`_.
 
diff --git a/sklearn/linear_model/_ransac.py b/sklearn/linear_model/_ransac.py
index 71d2ca291fcbe..2a5729a1123d7 100644
--- a/sklearn/linear_model/_ransac.py
+++ b/sklearn/linear_model/_ransac.py
@@ -86,11 +86,20 @@ class RANSACRegressor(
     min_samples : int (>= 1) or float ([0, 1]), default=None
         Minimum number of samples chosen randomly from original data. Treated
         as an absolute number of samples for `min_samples >= 1`, treated as a
-        relative number `ceil(min_samples * X.shape[0]`) for
+        relative number `ceil(min_samples * X.shape[0])` for
         `min_samples < 1`. This is typically chosen as the minimal number of
         samples necessary to estimate the given `base_estimator`. By default a
         ``sklearn.linear_model.LinearRegression()`` estimator is assumed and
-        `min_samples` is chosen as ``X.shape[1] + 1``.
+        `min_samples` is chosen as ``X.shape[1] + 1``. This parameter is highly
+        dependent upon the model, so if a `base_estimator` other than
+        :class:`linear_model.LinearRegression` is used, the user is
+        encouraged to provide a value.
+
+        .. deprecated :: 1.0
+           Not setting `min_samples` explicitly will raise an error in version
+           1.2 for model other than
+           :class:`~sklearn.linear_model.LinearRegression`. To keep the old
+           default behavior, set `min_samples=X.shape[1] + 1` explicitly.
 
     residual_threshold : float, default=None
         Maximum residual for a data sample to be classified as an inlier.
@@ -289,7 +298,15 @@ def fit(self, X, y, sample_weight=None):
             base_estimator = LinearRegression()
 
         if self.min_samples is None:
-            # assume linear model by default
+            if not isinstance(base_estimator, LinearRegression):
+                # FIXME: in 1.2, turn this warning into an error
+                warnings.warn(
+                    "From version 1.2, `max_samples` needs to be explicitely "
+                    "set otherwise an error will be raised. To keep the "
+                    "current behavior, you need to set `max_samples` to "
+                    f"`X.shape[1] + 1 that is {X.shape[1] + 1}",
+                    FutureWarning,
+                )
             min_samples = X.shape[1] + 1
         elif 0 < self.min_samples < 1:
             min_samples = np.ceil(self.min_samples * X.shape[0])
diff --git a/sklearn/linear_model/tests/test_ransac.py b/sklearn/linear_model/tests/test_ransac.py
index 340a950b29425..4f0402597be8e 100644
--- a/sklearn/linear_model/tests/test_ransac.py
+++ b/sklearn/linear_model/tests/test_ransac.py
@@ -8,7 +8,7 @@
 from sklearn.utils import check_random_state
 from sklearn.utils._testing import assert_allclose
 from sklearn.datasets import make_regression
-from sklearn.linear_model import LinearRegression, RANSACRegressor
+from sklearn.linear_model import LinearRegression, RANSACRegressor, Ridge
 from sklearn.linear_model import OrthogonalMatchingPursuit
 from sklearn.linear_model._ransac import _dynamic_max_trials
 from sklearn.exceptions import ConvergenceWarning
@@ -358,6 +358,10 @@ def test_ransac_min_n_samples():
     ransac_estimator7 = RANSACRegressor(
         base_estimator, min_samples=X.shape[0] + 1, residual_threshold=5, random_state=0
     )
+    # GH #19390
+    ransac_estimator8 = RANSACRegressor(
+        Ridge(), min_samples=None, residual_threshold=5, random_state=0
+    )
 
     ransac_estimator1.fit(X, y)
     ransac_estimator2.fit(X, y)
@@ -383,6 +387,10 @@ def test_ransac_min_n_samples():
     with pytest.raises(ValueError):
         ransac_estimator7.fit(X, y)
 
+    err_msg = "From version 1.2, `max_samples` needs to be explicitely set"
+    with pytest.warns(FutureWarning, match=err_msg):
+        ransac_estimator8.fit(X, y)
+
 
 def test_ransac_multi_dimensional_targets():
 
@@ -574,9 +582,10 @@ def test_ransac_fit_sample_weight():
     # check that if base_estimator.fit doesn't support
     # sample_weight, raises error
     base_estimator = OrthogonalMatchingPursuit()
-    ransac_estimator = RANSACRegressor(base_estimator)
+    ransac_estimator = RANSACRegressor(base_estimator, min_samples=10)
 
-    with pytest.raises(ValueError):
+    err_msg = f"{base_estimator.__class__.__name__} does not support sample_weight."
+    with pytest.raises(ValueError, match=err_msg):
         ransac_estimator.fit(X, y, weights)
 
 
diff --git a/sklearn/utils/estimator_checks.py b/sklearn/utils/estimator_checks.py
index ff853be22f663..f76e1c5c40ef1 100644
--- a/sklearn/utils/estimator_checks.py
+++ b/sklearn/utils/estimator_checks.py
@@ -26,7 +26,9 @@
 from ._testing import raises
 from . import is_scalar_nan
 
+from ..linear_model import LinearRegression
 from ..linear_model import LogisticRegression
+from ..linear_model import RANSACRegressor
 from ..linear_model import Ridge
 
 from ..base import (
@@ -354,7 +356,13 @@ def _construct_instance(Estimator):
     required_parameters = getattr(Estimator, "_required_parameters", [])
     if len(required_parameters):
         if required_parameters in (["estimator"], ["base_estimator"]):
-            if issubclass(Estimator, RegressorMixin):
+            # `RANSACRegressor` will raise an error with any model other
+            # than `LinearRegression` if we don't fix `min_samples` parameter.
+            # For common test, we can enforce using `LinearRegression` that
+            # is the default estimator in `RANSACRegressor` instead of `Ridge`.
+            if issubclass(Estimator, RANSACRegressor):
+                estimator = Estimator(LinearRegression())
+            elif issubclass(Estimator, RegressorMixin):
                 estimator = Estimator(Ridge())
             else:
                 estimator = Estimator(LogisticRegression(C=1))


Code review comments:
diff_hunk: @@ -289,7 +298,15 @@ def fit(self, X, y, sample_weight=None):
             base_estimator = LinearRegression()
 
         if self.min_samples is None:
-            # assume linear model by default
+            if not isinstance(base_estimator, LinearRegression):
+                # FIXME: in 1.2, turn this warning into an error
+                warnings.warn(
+                    "From version 1.2, `max_samples` needs to be explicitely "
path: sklearn/linear_model/_ransac.py
text: @user1:
```suggestion
                    "From version 1.2, `min_samples` needs to be explicitely "
```

@user2:
whoops :)

diff_hunk: @@ -383,6 +387,10 @@ def test_ransac_min_n_samples():
     with pytest.raises(ValueError):
         ransac_estimator7.fit(X, y)
 
+    err_msg = "From version 1.2, `max_samples` needs to be explicitely set"
path: sklearn/linear_model/tests/test_ransac.py
text: @user1:
```suggestion
    err_msg = "From version 1.2, `min_samples` needs to be explicitely set"
```

diff_hunk: @@ -289,7 +298,15 @@ def fit(self, X, y, sample_weight=None):
             base_estimator = LinearRegression()
 
         if self.min_samples is None:
-            # assume linear model by default
+            if not isinstance(base_estimator, LinearRegression):
+                # FIXME: in 1.2, turn this warning into an error
+                warnings.warn(
+                    "From version 1.2, `max_samples` needs to be explicitely "
+                    "set otherwise an error will be raised. To keep the "
+                    "current behavior, you need to set `max_samples` to "
path: sklearn/linear_model/_ransac.py
text: @user1:
```suggestion
                    "current behavior, you need to set `min_samples` to "
```

diff_hunk: @@ -86,11 +86,20 @@ class RANSACRegressor(
     min_samples : int (>= 1) or float ([0, 1]), default=None
         Minimum number of samples chosen randomly from original data. Treated
         as an absolute number of samples for `min_samples >= 1`, treated as a
-        relative number `ceil(min_samples * X.shape[0]`) for
+        relative number `ceil(min_samples * X.shape[0])` for
         `min_samples < 1`. This is typically chosen as the minimal number of
         samples necessary to estimate the given `base_estimator`. By default a
         ``sklearn.linear_model.LinearRegression()`` estimator is assumed and
-        `min_samples` is chosen as ``X.shape[1] + 1``.
+        `min_samples` is chosen as ``X.shape[1] + 1``. This parameter is highly
+        dependent upon the model, so if a `base_estimator` other than
+        :class:`linear_model.LinearRegression` is used, the user is
+        encouraged to provide a value.
+
+        .. deprecated :: 1.0
+           Not setting `min_samples` explicitly will raise an error in version
+           1.2 for model other than
path: sklearn/linear_model/_ransac.py
text: @user1:
Nit: grammar

```suggestion
           1.2 for models other than
```

diff_hunk: @@ -417,6 +417,11 @@ Changelog
 - |Enhancement| Validate user-supplied gram matrix passed to linear models
   via the `precompute` argument. :pr:`19004` by :user:`Adam Midvidy <amidvidy>`.
 
+- |API| Raise a warning in :class:`linear_model.RANSACRegressor` that from
+  version 1.2, `max_samples` need to be set explicitly for model other than
path: doc/whats_new/v1.0.rst
text: @user1:
```suggestion
  version 1.2, `min_samples` need to be set explicitly for model other than
```