scikit-learn · glemaitre · Feb 24, 2023 · Feb 22, 2023 · Feb 22, 2023 · Feb 23, 2023
diff --git a/doc/whats_new/v1.2.rst b/doc/whats_new/v1.2.rst
@@ -57,6 +57,13 @@ Changelog
  and :class:`ensemble.BaggingRegressor`.
  :pr:`25477` by :user:`Tim Head <betatim>`.

+:mod:`sklearn.feature_selection`
+................................
+
+- |Fix| Fixed a regression where a negative `tol` would not be accepted any more by
+  :class:`feature_selection.SequentialFeatureSelector`.
+  :pr:`25664` by :user:`Jérémie du Boisberranger <jeremiedbb>`.
+
 :mod:`sklearn.isotonic`
 .......................


diff --git a/sklearn/feature_selection/_sequential.py b/sklearn/feature_selection/_sequential.py
@@ -57,6 +57,11 @@ class SequentialFeatureSelector(SelectorMixin, MetaEstimatorMixin, BaseEstimator
    tol : float, default=None
        If the score is not incremented by at least `tol` between two
        consecutive feature additions or removals, stop adding or removing.
+
+        `tol` can be negative when removing features using `direction="backward"`.
+        It can be useful to reduce the number of features at the cost of a small
+        decrease in the score.
+
        `tol` is enabled only when `n_features_to_select` is `"auto"`.

        .. versionadded:: 1.1
@@ -153,7 +158,7 @@ class SequentialFeatureSelector(SelectorMixin, MetaEstimatorMixin, BaseEstimator
            Interval(Integral, 0, None, closed="neither"),
            Hidden(None),
        ],
-        "tol": [None, Interval(Real, 0, None, closed="neither")],
+        "tol": [None, Interval(Real, None, None, closed="neither")],
        "direction": [StrOptions({"forward", "backward"})],
        "scoring": [None, StrOptions(set(get_scorer_names())), callable],
        "cv": ["cv_object"],
@@ -250,6 +255,9 @@ def fit(self, X, y=None):
        elif isinstance(self.n_features_to_select, Real):
            self.n_features_to_select_ = int(n_features * self.n_features_to_select)

+        if self.tol is not None and self.tol < 0 and self.direction == "forward":
+            raise ValueError("tol must be positive when doing forward selection")
+
        cloned_estimator = clone(self.estimator)

        # the current mask corresponds to the set of features:

diff --git a/sklearn/feature_selection/tests/test_sequential.py b/sklearn/feature_selection/tests/test_sequential.py
@@ -278,3 +278,39 @@ def test_no_y_validation_model_fit(y):

    with pytest.raises((TypeError, ValueError)):
        sfs.fit(X, y)
+
+
+def test_forward_neg_tol_error():
+    """Check that we raise an error when tol<0 and direction='forward'"""
+    X, y = make_regression(n_features=10, random_state=0)
+    sfs = SequentialFeatureSelector(
+        LinearRegression(),
+        n_features_to_select="auto",
+        direction="forward",
+        tol=-1e-3,
+    )
+
+    with pytest.raises(ValueError, match="tol must be positive"):
+        sfs.fit(X, y)
+
+
+def test_backward_neg_tol():
+    """Check that SequentialFeatureSelector works negative tol
+
+    non-regression test for #25525
+    """
+    X, y = make_regression(n_features=10, random_state=0)
+    lr = LinearRegression()
+    initial_score = lr.fit(X, y).score(X, y)
+
+    sfs = SequentialFeatureSelector(
+        lr,
+        n_features_to_select="auto",
+        direction="backward",
+        tol=-1e-3,
+    )
+    Xr = sfs.fit_transform(X, y)
+    new_score = lr.fit(Xr, y).score(Xr, y)
+
+    assert 0 < sfs.get_support().sum() < X.shape[1]
+    assert new_score < initial_score