FIX clone to handle dict and GridSearchCV changing original params (scikit-learn#26786)

Co-authored-by: Thomas J. Fan <[email protected]>

Author: 2 people

doc/whats_new/v1.4.rst

@@ -51,6 +51,9 @@ Changelog
   passed to the ``fit`` method of the the estimator. :pr:`26506` by `Adrin
   Jalali`_.
 
+- |Enhancement| :func:`base.clone` now supports `dict` as input and creates a
+  copy. :pr:`26786` by `Adrin Jalali`_.
+
 :mod:`sklearn.decomposition`
 ............................
 
@@ -85,6 +88,15 @@ Changelog
   to :ref:`metadata routing user guide <metadata_routing>`. :pr:`26789` by
   `Adrin Jalali`_.
 
+:mod:`sklearn.model_selection`
+..............................
+
+- |Fix| :class:`model_selection.GridSearchCV`,
+  :class:`model_selection.RandomizedSearchCV`, and
+  :class:`model_selection.HalvingGridSearchCV` now don't change the given
+  object in the parameter grid if it's an estimator. :pr:`26786` by `Adrin
+  Jalali`_.
+
 :mod:`sklearn.tree`
 ...................
 

sklearn/base.py

@@ -80,8 +80,9 @@ def _clone_parametrized(estimator, *, safe=True):
     """Default implementation of clone. See :func:`sklearn.base.clone` for details."""
 
     estimator_type = type(estimator)
-    # XXX: not handling dictionaries
-    if estimator_type in (list, tuple, set, frozenset):
+    if estimator_type is dict:
+        return {k: clone(v, safe=safe) for k, v in estimator.items()}
+    elif estimator_type in (list, tuple, set, frozenset):
         return estimator_type([clone(e, safe=safe) for e in estimator])
     elif not hasattr(estimator, "get_params") or isinstance(estimator, type):
         if not safe:

sklearn/model_selection/_search.py

@@ -923,11 +923,14 @@ def evaluate_candidates(candidate_params, cv=None, more_results=None):
             self.best_params_ = results["params"][self.best_index_]
 
         if self.refit:
-            # 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_)
+            # here we clone the estimator as well as the parameters, since
+            # sometimes the parameters themselves might be estimators, e.g.
+            # when we search over different estimators in a pipeline.
+            # ref: https://github.com/scikit-learn/scikit-learn/pull/26786
+            self.best_estimator_ = clone(base_estimator).set_params(
+                **clone(self.best_params_, safe=False)
             )
+
             refit_start_time = time.time()
             if y is not None:
                 self.best_estimator_.fit(X, y, **fit_params)

sklearn/model_selection/_validation.py

@@ -726,14 +726,11 @@ def _fit_and_score(
     fit_params = _check_fit_params(X, fit_params, train)
 
     if parameters is not None:
-        # clone after setting parameters in case any parameters
-        # are estimators (like pipeline steps)
-        # because pipeline doesn't clone steps in fit
-        cloned_parameters = {}
-        for k, v in parameters.items():
-            cloned_parameters[k] = clone(v, safe=False)
-
-        estimator = estimator.set_params(**cloned_parameters)
+        # here we clone the parameters, since sometimes the parameters
+        # themselves might be estimators, e.g. when we search over different
+        # estimators in a pipeline.
+        # ref: https://github.com/scikit-learn/scikit-learn/pull/26786
+        estimator = estimator.set_params(**clone(parameters, safe=False))
 
     start_time = time.time()
 

sklearn/model_selection/tests/test_search.py

@@ -22,6 +22,7 @@
     make_multilabel_classification,
 )
 from sklearn.ensemble import HistGradientBoostingClassifier
+from sklearn.experimental import enable_halving_search_cv  # noqa
 from sklearn.impute import SimpleImputer
 from sklearn.linear_model import LinearRegression, Ridge, SGDClassifier
 from sklearn.metrics import (
@@ -38,6 +39,7 @@
     GridSearchCV,
     GroupKFold,
     GroupShuffleSplit,
+    HalvingGridSearchCV,
     KFold,
     LeaveOneGroupOut,
     LeavePGroupsOut,
@@ -2420,3 +2422,31 @@ def test_search_cv_verbose_3(capsys, return_train_score):
     else:
         match = re.findall(r"score=[\d\.]+", captured)
     assert len(match) == 3
+
+
+@pytest.mark.parametrize(
+    "SearchCV, param_search",
+    [
+        (GridSearchCV, "param_grid"),
+        (RandomizedSearchCV, "param_distributions"),
+        (HalvingGridSearchCV, "param_grid"),
+    ],
+)
+def test_search_estimator_param(SearchCV, param_search):
+    # test that SearchCV object doesn't change the object given in the parameter grid
+    X, y = make_classification(random_state=42)
+
+    params = {"clf": [LinearSVC(dual="auto")], "clf__C": [0.01]}
+    orig_C = params["clf"][0].C
+
+    pipe = Pipeline([("trs", MinimalTransformer()), ("clf", None)])
+
+    param_grid_search = {param_search: params}
+    gs = SearchCV(pipe, refit=True, cv=2, scoring="accuracy", **param_grid_search).fit(
+        X, y
+    )
+
+    # testing that the original object in params is not changed
+    assert params["clf"][0].C == orig_C
+    # testing that the GS is setting the parameter of the step correctly
+    assert gs.best_estimator_.named_steps["clf"].C == 0.01

sklearn/tests/test_base.py

@@ -184,6 +184,13 @@ def test_clone_nan():
     assert clf.empty is clf2.empty
 
 
+def test_clone_dict():
+    # test that clone creates a clone of a dict
+    orig = {"a": MyEstimator()}
+    cloned = clone(orig)
+    assert orig["a"] is not cloned["a"]
+
+
 def test_clone_sparse_matrices():
     sparse_matrix_classes = [
         cls