Repository: scikit-learn/scikit-learn
Pull request: [MRG+1] fix _BaseComposition._set_params with nested parameters
Fixes #9944.

This seemed the simplest fix.

This kind of means that the protection in ``_BaseComposition._set_params`` is not enough.
We could make that method work by grouping the things in ``BaseEstimator.set_params`` according to their prefixes and call ``set_params`` only once per prefix. That seems like a slightly cleaner solution, but not sure it's worth the effort.

This will go away in Python3.6 as iteration is guaranteed to be ordered then.

Problem statement:
Pipeline in Pipeline seems to not work well with setting of parameters using `.set_params`
#### Description
Using Pipeline in Pipeline in GridSearchCV fails sometimes at random. Use a snippet of code below to reproduce (fails ~50% of the time). 

#### Steps/Code to Reproduce
```python
from sklearn.datasets import load_diabetes
from sklearn.model_selection import train_test_split, GridSearchCV
from sklearn.linear_model import Lasso
from sklearn.dummy import DummyRegressor
from sklearn.pipeline import Pipeline

X, y = load_diabetes(True)
X_train, X_test, y_train, y_test = train_test_split(X, y, train_size=0.75)

gscv = GridSearchCV(
    estimator=Pipeline([ # pipeline in a pipeline
        ('a', Pipeline([
            ('b', DummyRegressor())
        ]))
    ]),
    param_grid={
        'a__b__alpha':[0.1, 0.001],
        'a__b':[Lasso()],
    }
)

gscv.fit(X_train, y_train)
print(gscv.score(X_test, y_test))
```

#### Expected Results
The code should work without exceptions.

#### Actual Results
Sometimes I get an error of the form
```
...
File "/home/iaroslav/.local/lib/python3.5/site-packages/sklearn/pipeline.py", line 144, in set_params
    self._set_params('steps', **kwargs)
  File "/home/iaroslav/.local/lib/python3.5/site-packages/sklearn/utils/metaestimators.py", line 49, in _set_params
    super(_BaseComposition, self).set_params(**params)
  File "/home/iaroslav/.local/lib/python3.5/site-packages/sklearn/base.py", line 276, in set_params
    sub_object.set_params(**{sub_name: value})
  File "/home/iaroslav/.local/lib/python3.5/site-packages/sklearn/base.py", line 283, in set_params
    (key, self.__class__.__name__))
ValueError: Invalid parameter alpha for estimator DummyRegressor. Check the list of available parameters with `estimator.get_params().keys()`.
```

#### Reason for the issue

It appears that order in which parameters are set is random. Because of this, sometimes the values of `a__b__alpha` is set before the step `a__b` is set as such. See the code below.

#### Further code to reproduce
This raises same exception:
```python
from sklearn.datasets import load_diabetes
from sklearn.model_selection import train_test_split, GridSearchCV
from sklearn.linear_model import Lasso
from sklearn.dummy import DummyRegressor
from sklearn.pipeline import Pipeline

X, y = load_diabetes(True)
X_train, X_test, y_train, y_test = train_test_split(X, y, train_size=0.75)

model = Pipeline([ # pipeline in a pipeline
    ('a', Pipeline([
        ('b', DummyRegressor())
    ]))
])

model.set_params(**{
    'a__b':Lasso(),
    'a__b__alpha':[0.01],
})

model.fit(X_train, y_train)
```

#### Versions
Linux-4.10.0-37-generic-x86_64-with-Ubuntu-16.04-xenial
Python 3.5.2 (default, Aug 18 2017, 17:48:00) 
[GCC 5.4.0 20160609]
NumPy 1.13.3
SciPy 0.19.1
Scikit-Learn 0.19.0

#### Possible solution?
Maybe it would help to set parameters in order from shortest parameter name string to longest one. But maybe also looking more into Pipeline is necessary. 

Should one not use Pipeline in Pipeline? But could the issue translate also to some complex estimators, eg Pipeline in FeatureUnion in Pipeline?

P.S. Thanks for the awesome library.

Hints from issue discussion:
Lol I was trying to reproduce and couldn't, and I think I know why. I'm using Python3.6 where all dicts are ordered.
I think we need to make the iteration ordered in BaseEstimator.set_params, that should fix it.
Yup can confirm, works fine with python3.6 :) 
also: good catch! Btw, do you want a job?
As the author of `_BaseComposition._set_params`, I think this case should have been handled there. It should have set `a__b` before `a__b__alpha`. Both `b` and `b__alpha` should be passed to `Pipeline.set_params` (and thence to `_BaseComposition._set_params`), but only `b__alpha` should be forwarded onto `BaseEstimator.set_params`. Could you check whether/why this is not the case?
I see what's wrong. #9945 is the wrong fix (even if the determinism might
be good and fix this particular case).

More generally the issue is that BaseEstimator.set_params forwards one
parameter at a time, rather than forwarding all parameters with a
particular prefix at the same time. Fix welcome.

This is why the nested pipelines were necessary for the issue to occur. Thanks for finding it @iaroslav-ai: I used this kind of nested pipeline a lot (and created http://searchgrid.readthedocs.io/ in part to help me do so).

I've updated #9945 with an appropriate fix. Though perhaps I should have opened a new PR.

Commit message: Unify error messages

You can't determine even local parameters with only class name

Patch to review:
diff --git a/sklearn/base.py b/sklearn/base.py
index d97fe92ccdd47..b653b7149c373 100644
--- a/sklearn/base.py
+++ b/sklearn/base.py
@@ -5,6 +5,7 @@
 
 import copy
 import warnings
+from collections import defaultdict
 
 import numpy as np
 from scipy import sparse
@@ -248,26 +249,24 @@ def set_params(self, **params):
             # Simple optimization to gain speed (inspect is slow)
             return self
         valid_params = self.get_params(deep=True)
-        for key, value in six.iteritems(params):
-            split = key.split('__', 1)
-            if len(split) > 1:
-                # nested objects case
-                name, sub_name = split
-                if name not in valid_params:
-                    raise ValueError('Invalid parameter %s for estimator %s. '
-                                     'Check the list of available parameters '
-                                     'with `estimator.get_params().keys()`.' %
-                                     (name, self))
-                sub_object = valid_params[name]
-                sub_object.set_params(**{sub_name: value})
+
+        nested_params = defaultdict(dict)  # grouped by prefix
+        for key, value in params.items():
+            key, delim, sub_key = key.partition('__')
+            if key not in valid_params:
+                raise ValueError('Invalid parameter %s for estimator %s. '
+                                 'Check the list of available parameters '
+                                 'with `estimator.get_params().keys()`.' %
+                                 (key, self))
+
+            if delim:
+                nested_params[key][sub_key] = value
             else:
-                # simple objects case
-                if key not in valid_params:
-                    raise ValueError('Invalid parameter %s for estimator %s. '
-                                     'Check the list of available parameters '
-                                     'with `estimator.get_params().keys()`.' %
-                                     (key, self.__class__.__name__))
                 setattr(self, key, value)
+
+        for key, sub_params in nested_params.items():
+            valid_params[key].set_params(**sub_params)
+
         return self
 
     def __repr__(self):
diff --git a/sklearn/tests/test_base.py b/sklearn/tests/test_base.py
index 7ad0f20382657..580a4e2ecac9f 100644
--- a/sklearn/tests/test_base.py
+++ b/sklearn/tests/test_base.py
@@ -228,6 +228,24 @@ def test_set_params():
     #               bad__stupid_param=True)
 
 
+def test_set_params_passes_all_parameters():
+    # Make sure all parameters are passed together to set_params
+    # of nested estimator. Regression test for #9944
+
+    class TestDecisionTree(DecisionTreeClassifier):
+        def set_params(self, **kwargs):
+            super(TestDecisionTree, self).set_params(**kwargs)
+            # expected_kwargs is in test scope
+            assert kwargs == expected_kwargs
+            return self
+
+    expected_kwargs = {'max_depth': 5, 'min_samples_leaf': 2}
+    for est in [Pipeline([('estimator', TestDecisionTree())]),
+                GridSearchCV(TestDecisionTree(), {})]:
+        est.set_params(estimator__max_depth=5,
+                       estimator__min_samples_leaf=2)
+
+
 def test_score_sample_weight():
 
     rng = np.random.RandomState(0)
diff --git a/sklearn/tests/test_pipeline.py b/sklearn/tests/test_pipeline.py
index d1d62f80e51a5..36a1e2a37ee0a 100644
--- a/sklearn/tests/test_pipeline.py
+++ b/sklearn/tests/test_pipeline.py
@@ -24,10 +24,11 @@
 from sklearn.base import clone, BaseEstimator
 from sklearn.pipeline import Pipeline, FeatureUnion, make_pipeline, make_union
 from sklearn.svm import SVC
-from sklearn.linear_model import LogisticRegression
+from sklearn.linear_model import LogisticRegression, Lasso
 from sklearn.linear_model import LinearRegression
 from sklearn.cluster import KMeans
 from sklearn.feature_selection import SelectKBest, f_classif
+from sklearn.dummy import DummyRegressor
 from sklearn.decomposition import PCA, TruncatedSVD
 from sklearn.datasets import load_iris
 from sklearn.preprocessing import StandardScaler
@@ -289,7 +290,7 @@ def test_pipeline_raise_set_params_error():
                  'with `estimator.get_params().keys()`.')
 
     assert_raise_message(ValueError,
-                         error_msg % ('fake', 'Pipeline'),
+                         error_msg % ('fake', pipe),
                          pipe.set_params,
                          fake='nope')
 
@@ -863,6 +864,20 @@ def test_step_name_validation():
                                  [[1]], [1])
 
 
+def test_set_conditional_params():
+    estimator = Pipeline([
+        ('a', Pipeline([
+            ('b', DummyRegressor())
+        ]))
+    ])
+    params = {
+        'a__b__alpha': 0.001,
+        'a__b': Lasso(),
+    }
+
+    estimator.set_params(**params)
+
+
 def test_pipeline_wrong_memory():
     # Test that an error is raised when memory is not a string or a Memory
     # instance


Code review comments:
diff_hunk: @@ -863,6 +864,20 @@ def test_step_name_validation():
                                  [[1]], [1])
 
 
+def test_set_conditional_params():
+    estimator = Pipeline([
+        ('a', Pipeline([
+            ('b', DummyRegressor())
+        ]))
+    ])
+    params = {
path: sklearn/tests/test_pipeline.py
text: @user1:
Should we use a `collections.OrderedDict` to make this test more future-proof?

@user2:
Shrug. I think it distorts the purpose of the test somewhat if we *only* test with an OrderedDict.

@user1:
I think this is a bit clearer (without some of the subtlety of python version-specific dict ordering):

```py
estimator.set_params(a__b__alpha=0.001, a__b=Lasso())
```