Repository: scikit-learn/scikit-learn
Pull request: [MRG] Fix GBDT init parameter when it's a pipeline
<!--
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/master/CONTRIBUTING.md#pull-request-checklist
-->

#### 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 #13466

#### What does this implement/fix? Explain your changes.

This PR fixes the support of the init estimator of GBDTs when init is a pipeline.

Note that pipeline do not support sample weights.

#### Any other comments?


<!--
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:
GradientBoostingRegressor initial estimator does not play together with Pipeline
Using a pipeline as the initial estimator of GradientBoostingRegressor doesn't work due to incompatible signatures.

```python
import sklearn
import sklearn.pipeline
import sklearn.ensemble
import sklearn.decomposition
import sklearn.linear_model
import numpy as np
init = sklearn.pipeline.make_pipeline(sklearn.decomposition.PCA(), sklearn.linear_model.ElasticNet())
model = sklearn.ensemble.GradientBoostingRegressor(init=init)
x = np.random.rand(12, 3)
y = np.random.rand(12)
model.fit(x, y)

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/Thomas/.local/miniconda3/envs/4cast/lib/python3.6/site-packages/sklearn/ensemble/gradient_boosting.py", line 1421, in fit
    self.init_.fit(X, y, sample_weight)
TypeError: fit() takes from 2 to 3 positional arguments but 4 were given
```
The signature of `Pipeline.fit` is

```python
# sklearn/pipeline.py
...
239 def fit(self, X, y=None, **fit_params):
...
```
which cannot be called with three positional arguments as above.

So I guess line 1421 in `sklearn/ensemble/gradient_boosting.py` should read
`self.init_.fit(X, y, sample_weight=sample_weight)` instead and the issue is solved. Right?

#### Versions
```python
>>> sklearn.show_versions()

System:
    python: 3.6.2 |Continuum Analytics, Inc.| (default, Jul 20 2017, 13:14:59)  [GCC 4.2.1 Compatible Apple LLVM 6.0 (clang-600.0.57)]
executable: /Users/Thomas/.local/miniconda3/envs/test/bin/python
   machine: Darwin-18.2.0-x86_64-i386-64bit

BLAS:
    macros: SCIPY_MKL_H=None, HAVE_CBLAS=None
  lib_dirs: /Users/Thomas/.local/miniconda3/envs/test/lib
cblas_libs: mkl_rt, pthread

Python deps:
       pip: 10.0.1
setuptools: 39.2.0
   sklearn: 0.20.2
     numpy: 1.16.1
     scipy: 1.2.0
    Cython: None
    pandas: 0.24.2
```

Hints from issue discussion:

Commit message: better handling

Patch to review:
diff --git a/sklearn/ensemble/gradient_boosting.py b/sklearn/ensemble/gradient_boosting.py
index ba089fc30cc3b..edc1ad3ef4c12 100644
--- a/sklearn/ensemble/gradient_boosting.py
+++ b/sklearn/ensemble/gradient_boosting.py
@@ -1484,7 +1484,7 @@ def fit(self, X, y, sample_weight=None, monitor=None):
             else:
                 try:
                     self.init_.fit(X, y, sample_weight=sample_weight)
-                except TypeError:
+                except (TypeError, ValueError):
                     if sample_weight_is_none:
                         self.init_.fit(X, y)
                     else:
diff --git a/sklearn/ensemble/tests/test_gradient_boosting.py b/sklearn/ensemble/tests/test_gradient_boosting.py
index dc9ec0c2338d0..389e45fec6b72 100644
--- a/sklearn/ensemble/tests/test_gradient_boosting.py
+++ b/sklearn/ensemble/tests/test_gradient_boosting.py
@@ -39,6 +39,8 @@
 from sklearn.exceptions import DataConversionWarning
 from sklearn.exceptions import NotFittedError
 from sklearn.dummy import DummyClassifier, DummyRegressor
+from sklearn.pipeline import make_pipeline
+from sklearn.linear_model import LinearRegression
 
 
 GRADIENT_BOOSTING_ESTIMATORS = [GradientBoostingClassifier,
@@ -1366,6 +1368,21 @@ def test_gradient_boosting_with_init(gb, dataset_maker, init_estimator):
         gb(init=init_est).fit(X, y, sample_weight=sample_weight)
 
 
+def test_gradient_boosting_with_init_pipeline():
+    # Check that the init estimator can be a pipeline (see issue #13466)
+
+    X, y = make_regression(random_state=0)
+    init = make_pipeline(LinearRegression())
+    gb = GradientBoostingRegressor(init=init)
+    gb.fit(X, y)
+
+    with pytest.raises(
+            ValueError,
+            match='The initial estimator Pipeline does not support sample '
+                  'weights'):
+        gb.fit(X, y, sample_weight=np.ones(X.shape[0]))
+
+
 @pytest.mark.parametrize('estimator, missing_method', [
     (GradientBoostingClassifier(init=LinearSVC()), 'predict_proba'),
     (GradientBoostingRegressor(init=OneHotEncoder()), 'predict')


Code review comments:
diff_hunk: @@ -1484,7 +1484,7 @@ def fit(self, X, y, sample_weight=None, monitor=None):
             else:
                 try:
                     self.init_.fit(X, y, sample_weight=sample_weight)
-                except TypeError:
+                except (TypeError, ValueError):
path: sklearn/ensemble/gradient_boosting.py
text: @user1:
I feel like here it should be actually more strict catching than loose. A lot of (if not most of) init param validation happens in fit, and they raise a `ValueError` if the parameters are not valid, and here the user would instead see a message complaining about `sample_weights` which would be irrelevant.

Wouldn't checking the signature, or the [appropriate] estimator tag be a better idea here?

@author:
>the user would instead see a message complaining about sample_weights which would be irrelevant.

Why is it irrelevant? This is precisely what this check is about.

>Wouldn't checking the signature, or the [appropriate] estimator tag be a better idea here? 

Yes I agree, but apparently using a try catch is preferred https://github.com/scikit-learn/scikit-learn/pull/12983#discussion_r254671832

EDIT: just saw that a `supports_sample_weight` tag is currently discussed https://github.com/scikit-learn/scikit-learn/issues/13438 but it's far from done

@user2:
It's not elegant but I'm okay with this.

@user1:
For example, NuSVR supports `sample_weights` on fit:

```
X, y = make_regression(random_state=0)
NuSVR(nu=1.5).fit(X, y, sample_weight=np.ones(X.shape[0]))
```
gives:

```
ValueError: nu <= 0 or nu > 1
```

But after this PR:

```
init = make_pipeline(NuSVR(nu=1.5))
gb = GradientBoostingRegressor(init=init)
gb.fit(X, y, sample_weight=np.ones(X.shape[0]))
```
gives:

```
ValueError: The initial estimator Pipeline does not support sample weights.
```

@author:
Ooh you were talking about the input checking of the init estimator, ok, good point.

@user2 do you think using `has_fit_param()` would be justified here? As far as I understand, this would only be a problem if a user passes a custom estimator which accepts `sample_weight` in `fit()` as a keyword args.

Another option would be to test the error message of the `ValueError` coming from a pipeline and only raise in this case?

@user2:
I don't see how has_fit_param helps here.

But perhaps we should raise a more equivocal error message ("could not fit init estimator with sample_weight") and use `raise from` to report the original exception

@author:
>I don't see how has_fit_param helps here.

This check is only supposed to check whether the init estimator supports samples_weights. I had to add `ValueError` for pipelines because unlike traditional estimators, they don't raise `TypeError`. As @user1 noted, now the check also catches ValueError coming from other reasons (namely input checking).

Using `has_fit_param` would avoid this, I think.

@user2:
> Using has_fit_param would avoid this, I think.

How so?

@author:
Using has_fit_param avoids using a try except