[FEATURE] Pass additional parameters to fit underlying estimator in EstimatorTransformer

[CarloLepelaars]

In EstimatorTransformer the underlying estimator is being fitted without the ability to pass along additional arguments to self.estimator_.fit.

This limits use cases for EstimatorTransformer. For example, if the underlying estimator is an XGBClassifier we would like to be able to pass eval_set to monitor validation performance and enable early stopping. This is currently not possible. Adding *args, **kwargs should fix this issue.

scikit-lego/sklego/meta/estimator_transformer.py

Line 31 in b4d087f

self.estimator_.fit(X, y)

[CarloLepelaars]

@koaning

In the future, please don't make a PR until the direction of the solution has been discussed in the issues.

I'll pick up the conversation there.

Ok, no problem! Will keep that in mind.

[koaning]

It's a bit of an sklearn antipattern to pass lots of settings via .fit(). Is XGBoost part of sklearn or a 3rd party lib?

[CarloLepelaars]

XGBoost is a 3rd party library maintained by dmlc.

I agree hyperparameters shouldn't be passed via .fit(). Unfortunately, some parameters like eval_set often can be passed only with .fit(). I believe sample_weight for scikit-learn estimators can also only be passed through .fit(), but am not completely sure.

Other example use cases include CatBoost parameters and Lightgbm parameters that can only be passed through .fit(). This libraries are also 3rd party, but very often used within scikit-learn Pipelines.

UPDATE: From looking at scikit-learn source code it seems sample_weight can only be passed through .fit() and never with class initialization parameters. Adding this as an optional parameter to EstimatorTransformer.fit() would not necessarily extend this library beyond scikit-learn. Understand the case for generalizing to *args, **kwargs is a bit trickier.

[koaning]

I'm a bit uneasy to extend this library beyond scikit-learn because the dependencies quickly start to stack up. @MBrouns what's your opinion on this?

[MBrouns]

sklearn does describe the use of kwargs in fit methods: https://scikit-learn.org/stable/developers/develop.html#fitting. I'm not sure I like varargs, but I don't see a lot of problems with varkwargs. I would like to see a test added in the PR though before accepting it

[koaning]

TIL.

Yeah so if scikit-learn supports **kwargs then I won't mind.

[CarloLepelaars]

Sounds great! I removed the *args option and added a test.

@koaning Can we reopen this PR or should I create a new one?

[koaning]

Either option is fine. Just as long as an issue is discussed before a solution is implemented.

[koaning]

Oh! And one more thing. If you're adding this behavior to the estimator transformer, could you also add it to the estimatorpredictor?

[CarloLepelaars]

Sure! Will check that out tomorrow. Do you mean pass **kwargs through the .transform method in EstimatorTransformer (+ a test case)?

[koaning]

This issue is about the .fit() method, no?

[CarloLepelaars]

Yes, but what exactly do you mean by estimatorpredictor otherwise? Don't see an EstimatorPredictor object in this repository (on the main branch).

[koaning]

Ah! Crud. My bad.

I was confused with the Grouped variant of the meta estimators. These come with a predictor variant.

Please ignore the previous comment.

[CarloLepelaars]

Aha, thanks for clearing that up! Then I think we are ready for the PR. Will create a fresh one.