DOC Update coef_ type for sparse linear models#34118
DOC Update coef_ type for sparse linear models#34118praful-srinivasan-027 wants to merge 7 commits into
Conversation
|
Hi everyone! Just a gentle bump on this PR whenever someone has a chance to take a look. I'd really appreciate any feedback. Thanks! |
|
Hi @GaelVaroquaux , @glemaitre, Just checking in on this small documentation fix since it's been 2 weeks. The CI is completely green and passing (30 tests passed, 8 skipped). If you have a quick moment to take a look or pass it along to whoever is currently tracking the doc queue, I'd really appreciate it. Thanks! |
StefanieSenger
left a comment
There was a problem hiding this comment.
thanks a lot for your contribution. I've left you two little comments.
Otherwise looks fine.
| By default, it will be created as a dense array, but can be turned to | ||
| sparse (CSR format) through :meth:`sparsify` (which can be beneficial | ||
| under L1 regularization when many coefficients are zero), and back to | ||
| dense through :meth:`densify`. | ||
|
|
There was a problem hiding this comment.
Why do you mention this here, but not in the other cases?
There was a problem hiding this comment.
There was originally a pr adding the docs addressing the same for logistic regression here #34093. I made these changes here to keep it consistent with this PR
There was a problem hiding this comment.
Sure, I know. What I meant is: why did you not not add a similar message in the other places to keep them consistent with #34093, too?
There was a problem hiding this comment.
mb, I've added the same explanatory text to the other affected estimators as well to keep the documentation consistent with #34093.
StefanieSenger
left a comment
There was a problem hiding this comment.
LGTM, thanks @praful-srinivasan-027.
| By default, it will be created as a dense array, but can be turned to | ||
| sparse (CSR format) through :meth:`sparsify` (which can be beneficial | ||
| under L1 regularization when many coefficients are zero), and back to | ||
| dense through :meth:`densify`. | ||
|
|
There was a problem hiding this comment.
Sure, I know. What I meant is: why did you not not add a similar message in the other places to keep them consistent with #34093, too?
| A list of class labels known to the classifier. | ||
|
|
||
| coef_ : ndarray or CSR matrix of shape (1, n_features) or (n_classes, n_features) | ||
| coef_ : ndarray or CSR array/matrix of shape (1, n_features) or (n_classes, \ |
There was a problem hiding this comment.
@jeremiedbb actually removed the array from here in #34093 (review), so this shouldn't be put back in here (and should probably also be removed from the other changes?).
There was a problem hiding this comment.
Oh, why was scipy.sparse.array as a possible type removed?
Since #31177 users can specify which type sparsify() returns with set_config.
|
Your last commit makes it worse, @praful-srinivasan-027. These don't all offer regularisation (which is mentioned in the explanation). Please remove the new additions. |
|
Thanks for the clarification. I've removed the L1-regularization-specific wording from the affected estimators and kept a generic explanation of the sparsify/densify behavior where applicable. The documentation now reflects the sparse coefficient support without making assumptions about regularization. |
Reference Issues/PRs
Fixes #34117
What does this implement/fix? Explain your changes.
Updates the
coef_attribute documentation for linear models supportingsparse coefficients through
sparsify().The documentation now mentions that
coef_can be either anndarrayor a sparse matrix for affected estimators such as SGD-based models,
Perceptron, PassiveAggressiveClassifier, and LogisticRegressionCV.
AI usage disclosure
I used AI assistance for:
Any other comments?