Repository: scikit-learn/scikit-learn
Pull request: FIX Improves feature names support for SelectFromModel + Est w/o names
<!--
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
-->
Fixes #21949

#### What does this implement/fix? Explain your changes.
`transform` will validate twice if the inner estimator supports `feature_names_in_` and also validates. This double validation already happens on `main`. 

#### Any other comments?
In a future PR, I think we need a way to configure `feature_names_in_` validation depending on if the delegated estimator supports `feature_names_in_`.  

<!--
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!
-->


Problem statement:
SelectFromModel function in scikit 1.0.1  does not work properly with catboost  and caching
### Describe the bug

Problem:  SelectFromModel function in scikit 1.01  does not work properly with catboost as it seems to delete column names,
making the return get_feature_names_out not return proper column names. This leads to additional problems when combining catboost and Scikit Learn in a pipeline and caching during hyperparameter optimization. For other models it works well.

Here is an reproducible example  and  the errror message from caching below it:

### Steps/Code to Reproduce

```python
from sklearn.svm import LinearSVR
from sklearn.datasets import load_iris
from catboost import CatBoostRegressor
import pandas as pd
import numpy as np
from sklearn.feature_selection import SelectFromModel

##################################################
#get the data and assign column names
##################################################

iris = load_iris()
X = pd.DataFrame(data= np.c_[iris['data']])
X.columns = ["a","b","c","d"]
y = iris.target

##########################################################
##test with catboost
##########################################################

#train the selector
selector = SelectFromModel(estimator=CatBoostRegressor())
selector.fit(X, y)
#show the selected columns
selector.get_feature_names_out()
#array(['x2', 'x3'], dtype=object) instead of "b" and "c"
#the oroginal column names have been replaced with x2 and x3

#if the selector is applied
selector.transform(X.iloc[1:2,:])
#I get the following warning
#X has feature names, but SelectFromModel was fitted without feature names

##############################################################
##Now the same with SVR, here it works
##############################################################

#train the selector
selector = SelectFromModel(estimator=LinearSVR())
selector.fit(X, y)

#the proper names are being returned
selector.get_feature_names_out()
#array(['d'], dtype=object) and "d" is a proper column name

#apply the selector
selector.transform(X.iloc[1:2,:])
#no warning is given
```
### Error message when using in hyperparameter tuning with caching

```pytb
File "C:\LocalTools\Miniconda\envs\my-rdkit-env\lib\site-packages\sklearn\pipeline.py", line 647, in transform
    Xt = transform.transform(Xt)
  File "C:\LocalTools\Miniconda\envs\my-rdkit-env\lib\site-packages\sklearn\feature_selection\_base.py", line 83, in transform
    X = self._validate_data(
  File "C:\LocalTools\Miniconda\envs\my-rdkit-env\lib\site-packages\sklearn\base.py", line 580, in _validate_data
    self._check_n_features(X, reset=reset)
  File "C:\LocalTools\Miniconda\envs\my-rdkit-env\lib\site-packages\sklearn\base.py", line 395, in _check_n_features
    raise ValueError(
ValueError: X has 290 features, but SelectFromModel is expecting 0 features as input.
```


### Actual Results

As no proper column names are being returned the caching does not seem to be able to match the names properly.

### Versions

System:
    python: 3.8.10 | packaged by conda-forge | (default, May 11 2021, 06:25:23) [MSC v.1916 64 bit (AMD64)]
executable: C:\LocalTools\Miniconda\envs\my-rdkit-env\python.exe
   machine: Windows-10-10.0.18363-SP0
Python dependencies:
          pip: 21.2.2
   setuptools: 57.4.0
      sklearn: 1.0.1
        numpy: 1.21.1
        scipy: 1.6.3
       Cython: None
       pandas: 1.3.1
   matplotlib: 3.4.2
       joblib: 1.1.0
threadpoolctl: 2.2.0
Built with OpenMP: True

Hints from issue discussion:
At a first glance, I think that the validation of `n_features_in_` in `SelectFromModel` is delegated to the model used for selecting. Since `catboost` does not implement the `n_features_in_` API, then it fails.

While I think that our implementation is right here, I think that we should have raised a warning at least until 1.1 without imposing this new API to third-party libraries. I assume that we have a similar issue for all the meta-estimator working similarly.

This should be addressed before the next bug fix release.
There are two issues here:

1. `get_feature_names_out` is incorrect when the inner estimator does not support `feature_names_in_`.
2. `transform` warns when the inner estimator does not support `feature_names_in_`.

I think the most lenient long term solution is to "delegate when possible". If the inner estimator does not support `feature_names_in_`, the meta-estimator will get the feature names and validates them in non-`fit` calls. WDYT?

Commit message: DOC Adds whats new PR number

Patch to review:
diff --git a/doc/whats_new/v1.0.rst b/doc/whats_new/v1.0.rst
index ce9069a6e8671..bc7cbb883626e 100644
--- a/doc/whats_new/v1.0.rst
+++ b/doc/whats_new/v1.0.rst
@@ -35,6 +35,12 @@ Changelog
   and :class:`decomposition.MiniBatchSparsePCA` to be convex and match the referenced
   article. :pr:`19210` by :user:`Jérémie du Boisberranger <jeremiedbb>`.
 
+:mod:`sklearn.feature_selection`
+................................
+
+- |Fix| Fixed :class:`feature_selection.SelectFromModel` to improve support
+  for estimators that set `feature_names_in_`. :pr:`21991` by `Thomas Fan`_.
+
 :mod:`sklearn.metrics`
 ......................
 
diff --git a/sklearn/feature_selection/_from_model.py b/sklearn/feature_selection/_from_model.py
index 55b7f6f070c9c..4ebc351eca085 100644
--- a/sklearn/feature_selection/_from_model.py
+++ b/sklearn/feature_selection/_from_model.py
@@ -265,8 +265,12 @@ def fit(self, X, y=None, **fit_params):
             raise NotFittedError("Since 'prefit=True', call transform directly")
         self.estimator_ = clone(self.estimator)
         self.estimator_.fit(X, y, **fit_params)
+
         if hasattr(self.estimator_, "feature_names_in_"):
             self.feature_names_in_ = self.estimator_.feature_names_in_
+        else:
+            self._check_feature_names(X, reset=True)
+
         return self
 
     @property
diff --git a/sklearn/feature_selection/tests/test_from_model.py b/sklearn/feature_selection/tests/test_from_model.py
index d8ae3de63a6a0..e65e9d2a4af88 100644
--- a/sklearn/feature_selection/tests/test_from_model.py
+++ b/sklearn/feature_selection/tests/test_from_model.py
@@ -428,3 +428,34 @@ def test_importance_getter(estimator, importance_getter):
     )
     selector.fit(data, y)
     assert selector.transform(data).shape[1] == 1
+
+
+class RandomForestNoFeatureNames(RandomForestClassifier):
+    def fit(self, X, y):
+        super().fit(X, y)
+        # Remove feature names
+        del self.feature_names_in_
+        return self
+
+
+def test_estimator_does_not_support_feature_names():
+    """SelectFromModel works with estimators that do not support feature_names_in_.
+
+    Non-regression test for #21949.
+    """
+    pytest.importorskip("pandas")
+    X, y = datasets.load_iris(as_frame=True, return_X_y=True)
+    all_feature_names = set(X.columns)
+
+    rf = RandomForestNoFeatureNames()
+    selector = SelectFromModel(rf).fit(X, y)
+
+    # selector learns the feature names itself
+    assert_array_equal(selector.feature_names_in_, X.columns)
+
+    feature_names_out = set(selector.get_feature_names_out())
+    assert feature_names_out < all_feature_names
+
+    with pytest.warns(None) as records:
+        selector.transform(X.iloc[1:3])
+    assert not [str(record.message) for record in records]


Code review comments:
diff_hunk: @@ -428,3 +428,34 @@ def test_importance_getter(estimator, importance_getter):
     )
     selector.fit(data, y)
     assert selector.transform(data).shape[1] == 1
+
+
+class RandomForestNoFeatureNames(RandomForestClassifier):
path: sklearn/feature_selection/tests/test_from_model.py
text: @user1:
I think that we should be using `MinimalClassifier` and `MinimalRegressor`.