Skip to content

Conversation

huangk10
Copy link
Contributor

@huangk10 huangk10 commented Dec 23, 2019

Reference Issues/PRs

See also #15953

What does this implement/fix? Explain your changes.

This allows the MultiOutputRegressor.fit to accept a **fit_params to custom estimator fit process.

Example:

class GradientBoostingRegressorWithFitParam(GradientBoostingRegressor):
    def fit(self, X, y, sample_weight=None, monitor=None, **fit_params):
        self.fit_params = fit_params
        super().fit(X, y, sample_weight=sample_weight)
        
X, y = datasets.make_regression(n_targets=3)
X_train, y_train = X[:50], y[:50]
X_test, y_test = X[50:], y[50:]

rgr = MultiOutputRegressor(GradientBoostingRegressorWithFitParam())
rgr.fit(X_train, y_train, customed_param=True)
y_pred = rgr.predict(X_test)

Any other comments?

To custom some of the estimators' fit processes, we have to redifine the estimators.fit functions with customed paramters besides sample_weight. Beacause the fit methods of most defined estimators in the sklearn package just have a sample_weight parameter.

@glemaitre
Copy link
Member

Be aware that we don't want to have any tight with LightGBM or some of their parameters. It might make sense that MultiOutputRegressor accepts a **fit_params but it should be decorrelated with LightGBM when coding in scikit-learn.

Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of changes. We also need a what's new entry.

@glemaitre glemaitre self-requested a review January 3, 2020 16:11
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you could just parametrize the test as below. Otherwise, LGTM

@huangk10 huangk10 requested a review from glemaitre January 6, 2020 00:59
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. ping @thomasjpfan

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @huangk10 !

@thomasjpfan thomasjpfan merged commit bdf1ae9 into scikit-learn:master Jan 6, 2020
panpiort8 pushed a commit to panpiort8/scikit-learn that referenced this pull request Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants