Open aidiss opened 2 years ago
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. Let me know if its in the right place and done in a right way.
Secondly, I wonder, what could be implementation for handling estimators that are inside pipelines. I guess we could traverse the estimators and look for the one that is supported? Or vice versa, skip the ones that are meta? Or, maybe it could be possible to tell specifically what kind of early stopping is used by the estimator?
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.