Repository: scikit-learn/scikit-learn
Pull request: MRG clone parameters in gridsearch etc
Fixed #10063 without going through the pain of #8350.

I don't see a case where this could change behavior, as we immediately call ``fit`` after setting the steps, but maybe I'm missing something.

This not only removes possible confusion of users (see #8350 on how the stored estimators are quite hard to interpret), it also saves us potentially *a lot* of memory (imagine grid-searching a neural net and storing the weights for each parameter setting).

Problem statement:
GridSearchCV saves all fitted estimator in cv_results['params'] when params are estimators
#### Description
I use GridSearchCV to optimize the hyperparameters of a pipeline. I set the param grid by inputing transformers or estimators at different steps of the pipeline, following the Pipeline documentation:

> A step’s estimator may be replaced entirely by setting the parameter with its name to another estimator, or a transformer removed by setting to None.

I couldn't figure why dumping cv_results_ would take so much memory on disk. It happens that cv_results_['params'] and all cv_results_['param_*'] objects contains fitted estimators, as much as there are points on my grid.

This bug should happen only when n_jobs = 1 (which is my usecase).

I don't think this is intended (else the arguments and attributes _refit_ and _best_\__estimator_ wouldn't be used).

My guess is that during the grid search, those estimator's aren't cloned before use (which could be a problem if using the same grid search several times, because estimators passed in the next grid would be fitted...).

#### Version: 0.19.0

Hints from issue discussion:
A solution could be to clone the estimator after set_param call in _fit_and_score, and return the new cloned estimator. However it would break any function that use _fit_and_score and expect to keep using the same estimator instance than passed in _fit_and_score..
Hmm... I agree this is a bit of an issue. But I think it is reflecting a
design issue with the estimator you are setting parameters of: it should
probably be cloning its parameters before fitting them if they did not
already come in fitted. Yes, our Pipeline and FeatureUnion are guilty of
this and it's a bit of a problem. We've been trying to work out how to
change it and not break too much code.

I'll have to think about what risks we'd take by cloning when setting
parameters. It's an interesting proposal. Certainly, users could have taken
advantage of this fact to allow estimators to accept pre-fitted models. And
it's long-standing behaviour, so I wouldn't want to break it without
warning...

I just spent hours trying to get to the bottom of some strange behavior and errors related to grabbing pipelines from cv_results_ and just realized this is likely the issue, when params are estimator objects they are all fitted and it causes weird errors when you try to reuse the pipeline.  In my case the first error was with StandardScaler() and getting the ValueError cannot broadcast... shape... yada.

I can give you a major use case for getting pipelines from cv_results_, I am using the nice GridSearchCV functionality to not only optimize estimator hyperparameters but also compare and optimize different pipeline step combos (thanks @jnothman for helping me with questions on implementing that a while back).  After it runs I interrogate cv_results_ to determine the best scores of each pipeline type combo and was grabbing those pipelines from cv_results_['params'] to run them each on held out test data to also get test scores.  When calling decision_function() or predict_proba() I was getting errors I didn't understand until now.

Either way it's not a work stoppage issue for me, I can simply use the original non-fitted estimator combos I passed in the param_grid to GridSearchCV in the first place.  Thanks though @fcharras making me realize what was wrong!

@hermidalc can you please open an issue with a reproducible example?
Student just came to me with this behavior being very confused. It looks like the estimators that are stored don't correspond to the same split, which is weird.

I'll have a minimum example soon.

I think cloning here would be the right thing to do, and I would call it a bug fix.
The only way a user could have used it here is using a meta-estimator with a prefit estimator, right?
I guess this would be a hacky work-around for being able to cross-validate something with a prefit estimator, but I feel like this is not a route we should encourage.

Maybe we can discuss this at the sprint?
Though on the other hand, this is part of the freezing discussion. If we can freeze, we can clone in the pipeline and then all is good...
```python
import numpy as np
import pandas as pd

from sklearn.linear_model import LinearRegression, Ridge
from sklearn.compose import make_column_transformer
from sklearn.preprocessing import OneHotEncoder 
from sklearn.impute import SimpleImputer
from sklearn.model_selection import train_test_split, GridSearchCV
from sklearn.pipeline import Pipeline

housing = pd.read_excel("http://www.amstat.org/publications/jse/v19n3/decock/AmesHousing.xls", nrows=500)

y = housing['SalePrice']
X = housing.drop(columns=['SalePrice', 'Order', 'PID'])

categorical = X.dtypes == object
X.loc[:, categorical] = X.loc[:, categorical].fillna("NaN")

X_train, X_test, y_train, y_test = train_test_split(X, y, random_state=0)

transform = make_column_transformer(
    (SimpleImputer(strategy="median"), ~categorical),
    (OneHotEncoder(handle_unknown="ignore"), categorical)
)

pipe = Pipeline([
    ('transformer', transform),
    ('regressor', LinearRegression())
])
param_grid = {'regressor': [LinearRegression(), Ridge()]}

grid = GridSearchCV(pipe, param_grid, cv=10)
grid.fit(X_train, y_train)

print("Number of Linear Regression Weights: {}" \
      .format(len(grid.cv_results_['param_regressor'][0].coef_)))

print("Number of Ridge Regression Weights: {}" \
      .format(len(grid.cv_results_['param_regressor'][1].coef_)))
```
> 258
> 260

I have no idea what's going on here, looks like the Imputer drops different numbers of columns or the OneHotEncoder sees different categories. But that shouldn't really happen, right? Also, this is deterministic.
Why shouldn't OneHotEncoder see different categories?

@jnothman because the order of CV folds is deterministic and the categories don't depend on whether ridge or lr is used, right?
Not sure if this helps, but I've been dealing with what seems to be the same, and have isolated the problem to the `clone` method, used [here](https://github.com/scikit-learn/scikit-learn/blob/7813f7efb/sklearn/model_selection/_validation.py#L779) inside of `cross_val_predict` and [here](https://github.com/scikit-learn/scikit-learn/blob/7813f7efb/sklearn/model_selection/_validation.py#L227) inside of cross_validate.

```
from sklearn.base import is_classifier, clone
pipe2 = clone(pipe)
first = pipe.fit_transform(train[features], train[target]).shape
second = pipe2.fit_transform(train[features], train[target]).shape
print(first)
>>> (16000, 245)
print(second)
>>> (16000, 13)
```

If this is relevant I can attempt to do an end-to-end example.

Commit message: add more tests

Patch to review:
diff --git a/sklearn/model_selection/_search.py b/sklearn/model_selection/_search.py
index 80e78e6b7f913..4b52c97901223 100644
--- a/sklearn/model_selection/_search.py
+++ b/sklearn/model_selection/_search.py
@@ -730,8 +730,10 @@ def evaluate_candidates(candidate_params):
             self.best_params_ = results["params"][self.best_index_]
 
         if self.refit:
-            self.best_estimator_ = clone(base_estimator).set_params(
-                **self.best_params_)
+            # we clone again after setting params in case some
+            # of the params are estimators as well.
+            self.best_estimator_ = clone(clone(base_estimator).set_params(
+                **self.best_params_))
             refit_start_time = time.time()
             if y is not None:
                 self.best_estimator_.fit(X, y, **fit_params)
diff --git a/sklearn/model_selection/_validation.py b/sklearn/model_selection/_validation.py
index e9374e23c48ca..d4618aa41bf42 100644
--- a/sklearn/model_selection/_validation.py
+++ b/sklearn/model_selection/_validation.py
@@ -488,7 +488,7 @@ def _fit_and_score(estimator, X, y, scorer, train, test, verbose,
 
     train_scores = {}
     if parameters is not None:
-        estimator.set_params(**parameters)
+        estimator = clone(estimator.set_params(**parameters))
 
     start_time = time.time()
 
diff --git a/sklearn/model_selection/tests/test_search.py b/sklearn/model_selection/tests/test_search.py
index 55cf58d2ac08b..fecd5a68d3d1a 100644
--- a/sklearn/model_selection/tests/test_search.py
+++ b/sklearn/model_selection/tests/test_search.py
@@ -63,7 +63,7 @@
 from sklearn.metrics import roc_auc_score
 from sklearn.impute import SimpleImputer
 from sklearn.pipeline import Pipeline
-from sklearn.linear_model import Ridge, SGDClassifier
+from sklearn.linear_model import Ridge, SGDClassifier, LinearRegression
 
 from sklearn.model_selection.tests.common import OneTimeSplitter
 
@@ -198,6 +198,21 @@ def test_grid_search():
     assert_raises(ValueError, grid_search.fit, X, y)
 
 
+def test_grid_search_pipeline_steps():
+    pipe = Pipeline([('regressor', LinearRegression())])
+    param_grid = {'regressor': [LinearRegression(), Ridge()]}
+    grid_search = GridSearchCV(pipe, param_grid, cv=2)
+    grid_search.fit(X, y)
+    regressor_results = grid_search.cv_results_['param_regressor']
+    assert isinstance(regressor_results[0], LinearRegression)
+    assert isinstance(regressor_results[1], Ridge)
+    assert not hasattr(regressor_results[0], 'coef_')
+    assert not hasattr(regressor_results[1], 'coef_')
+    # check that we didn't modify the parameter grid that was passed
+    assert not hasattr(param_grid['regressor'][0], 'coef_')
+    assert not hasattr(param_grid['regressor'][1], 'coef_')
+
+
 def check_hyperparameter_searcher_with_fit_params(klass, **klass_kwargs):
     X = np.arange(100).reshape(10, 10)
     y = np.array([0] * 5 + [1] * 5)


Code review comments:
diff_hunk: @@ -198,6 +198,21 @@ def test_grid_search():
     assert_raises(ValueError, grid_search.fit, X, y)
 
 
+def test_grid_search_pipeline_steps():
+    pipe = Pipeline([('regressor', LinearRegression())])
+    param_grid = {'regressor': [LinearRegression(), Ridge()]}
+    grid_search = GridSearchCV(pipe, param_grid, cv=2)
+    grid_search.fit(X, y)
+    regressor_results = grid_search.cv_results_['param_regressor']
+    assert isinstance(regressor_results[0], LinearRegression)
+    assert isinstance(regressor_results[1], Ridge)
+    assert not hasattr(regressor_results[0], 'coef_')
+    assert not hasattr(regressor_results[1], 'coef_')
+    # check that we didn't modify the parameter grid that was passed
+    assert not hasattr(param_grid['regressor'][0], 'coef_')
+    assert not hasattr(param_grid['regressor'][1], 'coef_')
path: sklearn/model_selection/tests/test_search.py
text: @user1:
`assert regressor_results[i] is not grid_search.best_estimator_` ?

@author:
well they are not fitted and ``best_estimator_`` is fitted. but I can add another check?

@user1:
yeah, doesn't hurt, and it specifically addresses https://github.com/scikit-learn/scikit-learn/issues/10063#issuecomment-535591894

@author:
done