-
Notifications
You must be signed in to change notification settings - Fork 51
Support sklearn.compose.TransformedTargetRegressor #240
base: master
Are you sure you want to change the base?
Conversation
Hey, this is a good catch. I think we would want to make this more generic if possible. We would eventually want to support other meta-estimators (including user-defined ones). Perhaps we can allow the user to just directly specify the early stop type. Also, I am not sure if we wouldn't need to change the logic related to early stopping in other places, too. Can you add a unit test for this? Thanks! |
Added a test. |
Secondly, I wonder, what could be implementation for handling estimators that are inside pipelines. |
@@ -101,6 +103,14 @@ def get_early_stop_type(estimator, early_stopping): | |||
|
|||
if not early_stopping: | |||
return EarlyStopping.NO_EARLY_STOP | |||
|
|||
if check_is_pipeline(estimator): | |||
for step_name, step in estimator.steps: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Three things:
- we shouldn't be only checking for regressors
- this is not foolproof & goes against duck typing principle of sklearn
- we can just assume that the last step of the pipeline is an estimator. You can't really make a pipeline with multiple estimators.
Hey @aidiss I took a look at the logic and I unfortunately don't think this will work. We may detect the early stop type correctly, but we are unable to apply it during actual training if the target transformer is present. It would require a more through refactoring of the code. I'll put this on the backlog, unless you would be willing to work on this. We would need to support both the case with a pipeline and without it. |
Currently TuneSearchCV fails when provided with LGBMRegressor provided wrapped inside TransformedTargerRegressor.
For example, this block would fail
Failure happens partly because of failure of tune_search.utils.get_early_stop_type to read the type of the estimator.
Note sure if this is the correct implementation. But it should not break anything.