[MRG] Fix GBDT init parameter when it's a pipeline #13472
Conversation
|
Perfect, thanks a lot! |
| @@ -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): | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 #12983 (comment)
EDIT: just saw that a supports_sample_weight tag is currently discussed #13438 but it's far from done
There was a problem hiding this comment.
It's not elegant but I'm okay with this.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ooh you were talking about the input checking of the init estimator, ok, good point.
@jnothman 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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 @adrinjalali noted, now the check also catches ValueError coming from other reasons (namely input checking).
Using has_fit_param would avoid this, I think.
There was a problem hiding this comment.
Using has_fit_param would avoid this, I think.
How so?
There was a problem hiding this comment.
Using has_fit_param avoids using a try except
| @@ -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): | |||
There was a problem hiding this comment.
It's not elegant but I'm okay with this.
|
Yes but it has false negatives
|
|
I updated the code. I hope it's clearer now. |
| if 'not enough values to unpack' in str(e): # pipeline | ||
| raise ValueError(msg) | ||
| else: # regular estimator whose input checking failed | ||
| raise e |
There was a problem hiding this comment.
Small nit:
except ValueError as e:
if 'not enough values to unpack' in str(e): # pipeline
raise ValueError(msg)
raise # regular estimator whose input checking failed| else: # regular estimator whose input checking failed | ||
| raise e | ||
| raise |
There was a problem hiding this comment.
Nit: Do not need the else here
There was a problem hiding this comment.
I personally prefer the whole if/else logic. It's clearer, it doesn't rely on the fact that the above block exits, and has a more functional flavor.
| except TypeError: # regular estimator without SW support | ||
| raise ValueError(msg) | ||
| except ValueError as e: | ||
| if 'not enough values to unpack' in str(e): # pipeline |
There was a problem hiding this comment.
I'd rather it if we improved the message for fit params missing __ in Pipeline, but okay
|
I think we'll be able to make it much better once we have a 'supports_sample_weight' tag |
Could you then add a |
|
@jnothman does your approval here stand? I'm not sure what you think about this one now. |
|
Let's merge and then change when #13534 is fixed. |
|
Thanks @NicolasHug! |
)" This reverts commit 7ab82a3.
)" This reverts commit 7ab82a3.
Reference Issues/PRs
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?