FIX #13117: Remove multiple output from LinearModelCV doc #13131
Conversation
albertotb
changed the title
| @@ -1084,7 +1084,7 @@ def fit(self, X, y): | |||
| to avoid unnecessary memory duplication. If y is mono-output, | |||
| X can be sparse. | |||
|
|
|||
| y : array-like, shape (n_samples,) or (n_samples, n_targets) | |||
| y : array-like, shape (n_samples,) | |||
There was a problem hiding this comment.
This is also used by MultiTask{Lasso,ElasticNet}CV so it is now more inaccurate than before!
There was a problem hiding this comment.
Then maybe instead of removing or (n_samples, n_targets), add to the description something like:
- shape (n_samples,) is valid for `ElasticNetCV` and `LassoCV`.
- shape (n_samples, n_targets) is valid for `MultiTaskElasticNetCV` and `MultitaskLassoCV`.
?
There was a problem hiding this comment.
I see. Now I understand why {Lasso,ElasticNet}CV do not support multiple outputs. To fix this I can think of two ways:
- different documentation for MultiTask/standard
- different classes LinearModelCV, MultiTaskLinearModelCV so multiple outputs are also supported.
Edit: @adrinjalali that also works, I think it is probably better to change n_targets by n_tasks, as in MultiTaskElasticNet
There was a problem hiding this comment.
@adrinjalali I just updated the doc with @adrinjalali suggestion. We can maybe open a new PR for extending multioutput to {Lasso,ElasticNet}CV
| @@ -1086,7 +1086,7 @@ def fit(self, X, y): | |||
|
|
|||
| y : array-like, shape | |||
| - (n_samples,), valid for `ElasticNetCV` and `LassoCV`. | |||
| - (n_samples, n_targets), valid for `MultiTaskElasticNetCV` and `MultitaskLassoCV`. | |||
| - (n_samples, n_tasks), valid for `MultiTaskElasticNetCV` and `MultitaskLassoCV`. | |||
There was a problem hiding this comment.
Is n_tasks used elsewhere in scikit-learn?
There was a problem hiding this comment.
Yes, it is the term used in MultiTaskElasticNet to refer to the extra dim of y, intercept_ and coef_
|
Maybe we should use n_targets or n_outputs in all of those to be consistent
with other parts of the library
|
|
@jnothman I believe the semantics are a little bit different between multiple targets and multiple tasks. For instance ElasticNet and MultiTaskElasticNet both accept a 2D |
|
It's still about model semantics not data structures. Fine either way.
|
| y : array-like, | ||
| - shape (n_samples,), valid for `ElasticNetCV` and `LassoCV`. | ||
| - shape (n_samples, n_tasks), valid for `MultiTaskElasticNetCV` | ||
| and `MultitaskLassoCV`. |
There was a problem hiding this comment.
this renders fine:
but it's not great that docstring of ElasticNetCV mentions estimators which are unrelated.... I am tempted to implement the fit in ElasticNetCV that will call super().fit just to have a good docstring...
There was a problem hiding this comment.
wouldn't we also need to implement fit methods in LassoCV, MultiTaskElasticNetCV and MultitaskLassoCV just for the doctring?
|
true. What do others think?
… |
|
potentially related to #13392 |
|
Any updates on this issue? Novice contributor looking to help out. |
Reference Issues/PRs
Fixes #13117
What does this implement/fix? Explain your changes.
Fixes documentation of class
LinearModelCV(), since it there is a check thatdoes not allow
yto have more than 1 dimension (multiple outputs)Any other comments?
I thinks this could be improved, since both
ElasticNet()andLasso()support multiple targets. I don't see a reason whyLinearModelCV()could not accept multiple targets as well.