[NOMRG] new warmstart API for GBDTs#15105
[NOMRG] new warmstart API for GBDTs#15105NicolasHug wants to merge 12 commits intoscikit-learn:masterfrom
Conversation
adrinjalali
left a comment
There was a problem hiding this comment.
Left a few notes as I was reading. Yeah this is what we had in mind IIRC (from the estimator's perspective).
The implementation in the pipeline would be trickier.
|
|
||
| # 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. |
adrinjalali
left a comment
There was a problem hiding this comment.
Once we decide on the API, I think we should put the warm startable params higher in the meta-estimator hierarchy (I think), but otherwise looks good.
And yeah we should write a slep for the new API.
| 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)