Repository: scikit-learn/scikit-learn
Pull request: ENH Allow `sample_weight` and other `fit_params` in RFE
#### Reference Issues/PRs

This PR takes over the work from  #7333 and fixes #7308
closes #7333

#### What does this implement/fix? Explain your changes.

This PR just continues the work from the original PR by implementing the changes on the current _main_ branch

The original description reads:

> Adds support for passing sample weights into RFE.fit() and having them used by the estimator's fit method.

> Adds a test for this with the iris dataset, where sample weights are used to give one class double its normal weight and compare that to doubling the samples in that class. The test passes if both these approaches produce the same feature ranking. This feature ranking is different from the one which arises when all samples have the same weight.

c/c: @ijpulidos 

Problem statement:
RFE/RFECV doesn't work with sample weights
As far as I can tell, `sklearn.feature_selection.RFE` has no way to pass sample weights to the estimator alongside the data.

I have fixed this in my code with:

``` diff
index bbe0cda..f5072b2 100644
--- a/sklearn/feature_selection/rfe.py
+++ b/sklearn/feature_selection/rfe.py
@@ -120,7 +120,7 @@ class RFE(BaseEstimator, MetaEstimatorMixin, SelectorMixin):
     def _estimator_type(self):
         return self.estimator._estimator_type

-    def fit(self, X, y):
+    def fit(self, X, y, **fit_params):
         """Fit the RFE model and then the underlying estimator on the selected
            features.

@@ -132,9 +132,9 @@ class RFE(BaseEstimator, MetaEstimatorMixin, SelectorMixin):
         y : array-like, shape = [n_samples]
             The target values.
         """
-        return self._fit(X, y)
+        return self._fit(X, y, **fit_params)

-    def _fit(self, X, y, step_score=None):
+    def _fit(self, X, y, step_score=None, **fit_params):
         X, y = check_X_y(X, y, "csc")
         # Initialization
         n_features = X.shape[1]
@@ -166,7 +166,7 @@ class RFE(BaseEstimator, MetaEstimatorMixin, SelectorMixin):
             if self.verbose > 0:
                 print("Fitting estimator with %d features." % np.sum(support_))

-            estimator.fit(X[:, features], y)
+            estimator.fit(X[:, features], y, **fit_params)

             # Get coefs
             if hasattr(estimator, 'coef_'):
```

Would this be a worthwhile contribution to scikit-learn?
#### Versions

<!--
Please run the following snippet and paste the output below.
import platform; print(platform.platform())
import sys; print("Python", sys.version)
import numpy; print("NumPy", numpy.__version__)
import scipy; print("SciPy", scipy.__version__)
import sklearn; print("Scikit-Learn", sklearn.__version__)
-->

``` python

In [1]: import platform; print(platform.platform())
Linux-3.13.0-63-generic-x86_64-with-Ubuntu-14.04-trusty

In [2]: import sys; print("Python", sys.version)
('Python', '2.7.6 (default, Jun 22 2015, 17:58:13) \n[GCC 4.8.2]')

In [3]: import numpy; print("NumPy", numpy.__version__)
('NumPy', '1.11.0')

In [4]: import scipy; print("SciPy", scipy.__version__)
('SciPy', '0.17.1')

In [5]: import sklearn; print("Scikit-Learn", sklearn.__version__)
('Scikit-Learn', '0.18.dev0')
```

<!-- Thanks for contributing! -->

TODO:

- [x] Add support for `sample_weight` in `RFE`
- [ ] Add support for `sample_weight` in `RFECV`

Hints from issue discussion:
yeah I think you can open a PR for that. You need to add tests, though.

Note that testing for sample weights usually involves checking that weights
correspond to repeating samples.

On 1 September 2016 at 03:33, Andreas Mueller notifications@github.com
wrote:

> yeah I think you can open a PR for that. You need to add tests, though.
> 
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> https://github.com/scikit-learn/scikit-learn/issues/7308#issuecomment-243839981,
> or mute the thread
> https://github.com/notifications/unsubscribe-auth/AAEz64VJO8sJbmnWKX_vPF61jQEWBmwwks5qlbrtgaJpZM4Jx1Lf
> .

Great, want to assign this to me @amueller? I'll sort out a PR with tests.

We don't use the "assignee" feature much, and for some reason it's not letting me assign. Go ahead and open a PR, reference this issue, then ping one of us after we've released 0.18: it's not likely to be looked at before then.

you can only "assign" contributors

Hey all, just an FIY, @ijpulidos and I worked on this over #20380 
@fbidu thank you so much for this! for what it's worth I think it would be fairly simple to add this also to RFECV (which just calls RFE), but I understand if that's outside of the scope of what you're working on (since you're already building/trying to merge)
@nathanwalker-sp you're welcome! Well, I agree with you and sure enough should be a simple change, but I'm not familiar with the practices adopted by this project to track these.

@glemaitre as the reviewer for my PR, what do you think? May I just go ahead and implement this there or is it better if we first merge #20380 and then create a new issue/PR pair to track RFECV?

Commit message: Update sklearn/feature_selection/_rfe.py

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

Patch to review:
diff --git a/doc/whats_new/v1.0.rst b/doc/whats_new/v1.0.rst
index ecb4b5972a669..ce252a502e28d 100644
--- a/doc/whats_new/v1.0.rst
+++ b/doc/whats_new/v1.0.rst
@@ -337,6 +337,11 @@ Changelog
   when the variance threshold is negative.
   :pr:`20207` by :user:`Tomohiro Endo <europeanplaice>`
 
+- |Enhancement| :func:`feature_selection.RFE.fit` accepts additional estimator
+  parameters that are passed directly to the estimator's `fit` method.
+  :pr:`20380` by :user:`Iván Pulido <ijpulidos>`, :user:`Felipe Bidu <fbidu>`,
+  :user:`Gil Rutter <g-rutter>`, and :user:`Adrin Jalali <adrinjalali>`.
+
 - |FIX| Fix a bug in :func:`isotonic.isotonic_regression` where the
   `sample_weight` passed by a user were overwritten during the fit.
   :pr:`20515` by :user:`Carsten Allefeld <allefeld>`.
diff --git a/sklearn/feature_selection/_rfe.py b/sklearn/feature_selection/_rfe.py
index 3471a0b939b59..5e6d421f537ba 100644
--- a/sklearn/feature_selection/_rfe.py
+++ b/sklearn/feature_selection/_rfe.py
@@ -192,7 +192,7 @@ def classes_(self):
         """
         return self.estimator_.classes_
 
-    def fit(self, X, y):
+    def fit(self, X, y, **fit_params):
         """Fit the RFE model and then the underlying estimator on the selected features.
 
         Parameters
@@ -203,14 +203,17 @@ def fit(self, X, y):
         y : array-like of shape (n_samples,)
             The target values.
 
+        **fit_params : dict
+            Additional parameters passed to the `fit` method of the estimator.
+
         Returns
         -------
         self : object
             Fitted estimator.
         """
-        return self._fit(X, y)
+        return self._fit(X, y, **fit_params)
 
-    def _fit(self, X, y, step_score=None):
+    def _fit(self, X, y, step_score=None, **fit_params):
         # Parameter step_score controls the calculation of self.scores_
         # step_score is not exposed to users
         # and is used when implementing RFECV
@@ -269,7 +272,7 @@ def _fit(self, X, y, step_score=None):
             if self.verbose > 0:
                 print("Fitting estimator with %d features." % np.sum(support_))
 
-            estimator.fit(X[:, features], y)
+            estimator.fit(X[:, features], y, **fit_params)
 
             # Get importance and rank them
             importances = _get_feature_importances(
@@ -296,7 +299,7 @@ def _fit(self, X, y, step_score=None):
         # Set final attributes
         features = np.arange(n_features)[support_]
         self.estimator_ = clone(self.estimator)
-        self.estimator_.fit(X[:, features], y)
+        self.estimator_.fit(X[:, features], y, **fit_params)
 
         # Compute step score when only n_features_to_select features left
         if step_score:
@@ -325,7 +328,7 @@ def predict(self, X):
         return self.estimator_.predict(self.transform(X))
 
     @if_delegate_has_method(delegate="estimator")
-    def score(self, X, y):
+    def score(self, X, y, **fit_params):
         """Reduce X to the selected features and return the score of the underlying estimator.
 
         Parameters
@@ -336,6 +339,11 @@ def score(self, X, y):
         y : array of shape [n_samples]
             The target values.
 
+        **fit_params : dict
+            Parameters to pass to the `score` method of the estimator.
+
+            .. versionadded:: 1.0
+
         Returns
         -------
         score : float
@@ -343,7 +351,7 @@ def score(self, X, y):
             features returned by `rfe.transform(X)` and `y`.
         """
         check_is_fitted(self)
-        return self.estimator_.score(self.transform(X), y)
+        return self.estimator_.score(self.transform(X), y, **fit_params)
 
     def _get_support_mask(self):
         check_is_fitted(self)
diff --git a/sklearn/feature_selection/tests/test_rfe.py b/sklearn/feature_selection/tests/test_rfe.py
index 190672ea248d3..d2e9ab16aafc5 100644
--- a/sklearn/feature_selection/tests/test_rfe.py
+++ b/sklearn/feature_selection/tests/test_rfe.py
@@ -8,6 +8,7 @@
 from numpy.testing import assert_array_almost_equal, assert_array_equal
 from scipy import sparse
 
+from sklearn.base import BaseEstimator, ClassifierMixin
 from sklearn.feature_selection import RFE, RFECV
 from sklearn.datasets import load_iris, make_friedman1
 from sklearn.metrics import zero_one_loss
@@ -108,6 +109,31 @@ def test_rfe():
     assert_array_almost_equal(X_r, X_r_sparse.toarray())
 
 
+def test_RFE_fit_score_params():
+    # Make sure RFE passes the metadata down to fit and score methods of the
+    # underlying estimator
+    class TestEstimator(BaseEstimator, ClassifierMixin):
+        def fit(self, X, y, prop=None):
+            if prop is None:
+                raise ValueError("fit: prop cannot be None")
+            self.svc_ = SVC(kernel="linear").fit(X, y)
+            self.coef_ = self.svc_.coef_
+            return self
+
+        def score(self, X, y, prop=None):
+            if prop is None:
+                raise ValueError("score: prop cannot be None")
+            return self.svc_.score(X, y)
+
+    X, y = load_iris(return_X_y=True)
+    with pytest.raises(ValueError, match="fit: prop cannot be None"):
+        RFE(estimator=TestEstimator()).fit(X, y)
+    with pytest.raises(ValueError, match="score: prop cannot be None"):
+        RFE(estimator=TestEstimator()).fit(X, y, prop="foo").score(X, y)
+
+    RFE(estimator=TestEstimator()).fit(X, y, prop="foo").score(X, y, prop="foo")
+
+
 @pytest.mark.parametrize("n_features_to_select", [-1, 2.1])
 def test_rfe_invalid_n_features_errors(n_features_to_select):
     clf = SVC(kernel="linear")


Code review comments:
diff_hunk: @@ -336,14 +339,19 @@ def score(self, X, y):
         y : array of shape [n_samples]
             The target values.
 
+        **fit_params : dict
+            Parameters to pass to the `score` method of the estimator.
path: sklearn/feature_selection/_rfe.py
text: @user1:
```suggestion
            Parameters to pass to the `score` method of the underlying estimator.
```