Repository: scikit-learn/scikit-learn
Pull request: [MRG+1] Fix/pipeline param error msg
#### Reference Issues/PRs
Fixes #13534 

#### What does this implement/fix? Explain your changes.
The error message for passing a sample_weight is unclear. A check has been introduced for when the step name is not found in the fit_params_steps that raises a TypeError.

#### Any other comments?
- Is the wording of the error clear enough?
- Will require adding a test case once approach is finalised

Problem statement:
improve error message when passing sample_weight to Pipeline
Many estimators take a parameter named `sample_weight`. `Pipeline` does not, since it wants its `fit` parameters to be prefixed by the step name with a `__` delimiter:

```pytb
>>> from sklearn.pipeline import make_pipeline
>>> from sklearn.linear_model import LogisticRegression
>>> clf = make_pipeline(LogisticRegression())
>>> clf.fit([[0], [0]], [0, 1], logisticregression__sample_weight=[1, 1])
Pipeline(memory=None,
     steps=[('logisticregression', LogisticRegression(C=1.0, class_weight=None, dual=False, fit_intercept=True,
          intercept_scaling=1, max_iter=100, multi_class='ovr', n_jobs=1,
          penalty='l2', random_state=None, solver='liblinear', tol=0.0001,
          verbose=0, warm_start=False))])
>>> clf.fit([[0], [0]], [0, 1], sample_weight=[1, 1])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/n/schwafs/home/joel/miniconda3/envs/scipy3k/lib/python3.6/site-packages/sklearn/pipeline.py", line 248, in fit
    Xt, fit_params = self._fit(X, y, **fit_params)
  File "/n/schwafs/home/joel/miniconda3/envs/scipy3k/lib/python3.6/site-packages/sklearn/pipeline.py", line 197, in _fit
    step, param = pname.split('__', 1)
ValueError: not enough values to unpack (expected 2, got 1)
```

This error message is not friendly enough. It should explicitly describe the correct format for passing `sample_weight` to a step in a Pipeline.

Hints from issue discussion:
There is some code in gradient boosting that checks for the current error message, so that should be updated to reflect a changed error message too.

(Arguably, this should be a TypeError, not a ValueError, since the user has passed the wrong parameter names, but I'm ambivalent to whether we fix that.)
I'd like to take this issue.
Go ahead, Osman

Commit message: Added test for sample_weight parameter error message

Patch to review:
diff --git a/sklearn/pipeline.py b/sklearn/pipeline.py
index d1d03656d2a62..411e5d4f40818 100644
--- a/sklearn/pipeline.py
+++ b/sklearn/pipeline.py
@@ -253,6 +253,13 @@ def _fit(self, X, y=None, **fit_params):
         fit_params_steps = {name: {} for name, step in self.steps
                             if step is not None}
         for pname, pval in fit_params.items():
+            if '__' not in pname:
+                raise TypeError(
+                    "Passing a parameter directly to a pipeline isn't "
+                    "supported. You can pass parameters to specific steps "
+                    "of your pipeline using the stepname__parameter "
+                    "format, e.g. `pipe.fit(X, y, "
+                    "logisticregression__sample_weight=sample_weight)`.")
             step, param = pname.split('__', 1)
             fit_params_steps[step][param] = pval
         Xt = X
diff --git a/sklearn/tests/test_pipeline.py b/sklearn/tests/test_pipeline.py
index ed81db747e20c..d77929c9ef357 100644
--- a/sklearn/tests/test_pipeline.py
+++ b/sklearn/tests/test_pipeline.py
@@ -1072,3 +1072,9 @@ def test_make_pipeline_memory():
     assert len(pipeline) == 2
 
     shutil.rmtree(cachedir)
+
+
+def test_pipeline_param_error():
+    clf = make_pipeline(LogisticRegression())
+    with pytest.raises(TypeError, match='stepname__parameter'):
+        clf.fit([[0], [0]], [0, 1], sample_weight=[1, 1])


Code review comments:
diff_hunk: @@ -253,6 +253,13 @@ def _fit(self, X, y=None, **fit_params):
         fit_params_steps = {name: {} for name, step in self.steps
                             if step is not None}
         for pname, pval in fit_params.items():
+            if '__' not in pname:
+                raise TypeError(
+                    "Passing a parameter directly to a pipeline isn't "
path: sklearn/pipeline.py
text: @user1:
I'm not sure if this is going to match the user's understanding.. Might be better to specify "Pipeline.fit does not accept the {} parameter. You can pass ..."