[NOMRG] new warmstart API for GBDTs #15105
Conversation
| @@ -106,10 +109,16 @@ def fit(self, X, y): | |||
|
|
|||
| rng = check_random_state(self.random_state) | |||
|
|
|||
| # For backward compat (for now) | |||
| if self.warm_start: | |||
| warnings.warn("warm_start parameter is deprecated", DeprecationWarning) | |||
There was a problem hiding this comment.
also mention how the user can fix it/what they should do instead.
There was a problem hiding this comment.
Sure this is just some sort of proof of concept.
In the specific case of the hist GBDT, we don't need to deprecate anything since the warm-start hasn't even been released yet :)
Are you sure? for the fit_param, we could just do e.g. and the |
|
I was thinking of allowing warm start, or refitting the pipeline, having changed a parameter anywhere, and only refit the steps after the changed one. So in a sense, pipeline can warm start with almost any of its parameters |
|
You mean, when doing I feel like this is yet another kind of warm-starting, mostly orthogonal to the kind of warm-starting that this API is concerned with? |
|
So for now, we forget about warm start on the pipeline for this PR, and we should test for metaestimators such as pipeline and Voting estimators. |
…rmstart_gbdt_gridsearch
…rmstart_gbdt_gridsearch
|
Are you happy with how it looks so far @adrinjalali ? (test failure is unrelated) I think we should write a SLEP for this. |
| def _warmstartable_parameters(self): | ||
| # This property exposes the _warmstartable_parameters attribute, e.g. | ||
| # ['+last_step_name__param'] | ||
| # We consider that only the last step can be warm-started. The first |
There was a problem hiding this comment.
Do we need to make this design choice? I may have asked this question before, but don't remember your logic behind it.
There was a problem hiding this comment.
A pipeline is either:
- a sequence of transformers
- a sequence of transformers + a predictor as the last step
It's safe to assume that transformers are not warm-startable. Maybe in the future one of them will be?? We can worry about that when that happens.
There was a problem hiding this comment.
You could have a word embedding transformer as a step, which very often is warm started. We may not have that inside sklearn, but the pipeline's API should support it, I think.
There was a problem hiding this comment.
I agree we should leave the possibility open, and make the code future-proof. This is one of the reasons why _warmstartable_parameters is a list.
But I don't think we should implement support for that, we have no use-case ATM.
Concretely, supporting warm-start for transformers right now is writing code that isn't used (that would require updating the _fit method that fits all the transformers)
|
@jnothman I'd appreciate your thoughts on this one. |
|
The benefits of this approach aren't clear to me, and we have enough SLEPs to deal with ATM. Closing, might re-open one day |
Ping @adrinjalali is this what was decided during the sprint?
(CI will break because I'm not catching the deprecation warnings)