Delegate choice of final model to sub class in LinearModelCV #17099
Delegate choice of final model to sub class in LinearModelCV #17099agramfort merged 5 commits intoscikit-learn:masterfrom
Conversation
NicolasHug
left a comment
There was a problem hiding this comment.
Thanks @mathurinm , it does look better
LGTM when green
| def _get_model(self): | ||
| return Lasso() |
There was a problem hiding this comment.
I feel like returning the class rather than an instance is more natural because we are going to use set_params on it later anyway
| def _get_model(self): | |
| return Lasso() | |
| def _get_model_class(self): | |
| return Lasso |
There was a problem hiding this comment.
so model.set_params(**common_params) becomes model = self._get_model_class(**common_params) ?
There was a problem hiding this comment.
@NicolasHug Actually we call model.get_params() before we set the parameters, so we need a "blank" instance available before set_params()
|
this should greatly facilitate subclassing in other packages. thanks @mathurinm for taking a stab at this ! |
|
also pinging @ogrisel who is interested in linear models :) |
| self.selection = selection | ||
|
|
||
| @abstractmethod | ||
| def _get_model(self): |
There was a problem hiding this comment.
To be consistent with scikit-learn conventions, I think _get_estimator would be more appropriate.
There was a problem hiding this comment.
If a class is returned, _get_estimator_class (or _get_estimator_type? class is probably better).
There was a problem hiding this comment.
I have renamed to _get_estimator(). We need to create a blank instance (to get its parameters) so I kept the design where we return an estimator and not the class.
ogrisel
left a comment
There was a problem hiding this comment.
LGTM as well. Few nitpicks / small suggestions:
ogrisel
left a comment
There was a problem hiding this comment.
I meant to approve instead of comment in the previous review.
|
All checks passed and three approvals: @scikit-learn/core-devs, ready to be merged? Thanks! |
|
3 approvals are enough so let's get this one in. thanks @mathurinm |
…learn#17099) * init * flake8 error by vscode autoformatting * remove returns * get_model > get_estimator * cosmit Tom + autoformat on save error again
…learn#17099) * init * flake8 error by vscode autoformatting * remove returns * get_model > get_estimator * cosmit Tom + autoformat on save error again
…learn#17099) * init * flake8 error by vscode autoformatting * remove returns * get_model > get_estimator * cosmit Tom + autoformat on save error again
Reference Issues/PRs
What does this implement/fix? Explain your changes.
The current design of the base class
LinearModelCVuses conditions based onl1_ratioto instantiate, in a hardcoded fashion, the model to be fitted after the best alpha has been determined by CV (egLassoforLassoCV,MultitaskElasticNetforMultitaskElasticNetCV)This is not a common design and it makes it difficult to subclass
LinearModelCVwhen implementingAnotherEstimatorCVWith the proposed PR, the model to instantiate is implemented by the sub class, in
_get_model(self)I also implemented
_is_multitask(self)to address a similar issue@agramfort
@NicolasHug do you have an opinion on this ?
Any other comments?