[MRG] Added missing sag_solver documentation #10172
Conversation
sklearn/linear_model/sag.py
Outdated
| Defaults to 1. | ||
|
|
||
| beta : float, optional | ||
| Constant that multiplies the l1 regularization term. Only applied |
There was a problem hiding this comment.
Can you replace the double spaces by a single space after each full stop.
There was a problem hiding this comment.
l1 -> L1 (to be consistent with your previous description)
|
does that mean we have an implementation of #8288 but no interface to it?! |
|
rephrased in terms of docs: can alpha and beta be != 0 at the same time for the current implementation or are they mutually exclusive? |
|
Sorry I missed @amueller's comments :/ |
|
I mean that doesn't mean it's not correct, I just wasn't sure. I'm not familiar with the implementation (and it wasn't entirely obvious to me). |
sklearn/linear_model/sag.py
Outdated
| @@ -131,7 +131,12 @@ def sag_solver(X, y, sample_weight=None, loss='log', alpha=1., beta=0., | |||
| *loss='multinomial'* | |||
|
|
|||
| alpha : float, optional | |||
| Constant that multiplies the regularization term. Defaults to 1. | |||
| Constant that multiplies the squared L2 regularization term. | |||
There was a problem hiding this comment.
L2 regularization term in the objective function (.5 * alpha * || W ||_F^2)
sklearn/linear_model/sag.py
Outdated
| Defaults to 1. | ||
|
|
||
| beta : float, optional | ||
| Constant that multiplies the L1 regularization term. Only applied |
There was a problem hiding this comment.
L2 regularization term in the objective function (beta * || W ||_1)
There was a problem hiding this comment.
I guess @arthurmensch meant L1 here and not L2.
|
@amueller yes basically we already have an elastic net regularization for |
|
oh wow! Why don't we expose that lol? |
|
@patrick1011 can you tackle @arthurmensch comments and then I think we can merge this one. |
|
@lesteve Ah sorry didn't realise I was to update anything. You would prefer me to use the comments he made as the documentation? If so can do! |
That'd be great indeed if you could use @arthurmensch comments for the docstrings. |
06e1f9a to
4bc025a
Compare
|
Hope that's okay now! |
|
I pushed a small tweak. Merging thanks a lot @patrick1011! |
Reference Issues/PRs
Fixes #10150 - beta parameter not documented in sag solver.
What does this implement/fix? Explain your changes.
Updated docs for
alphaandbetaparameters.Other comments
@arthurmensch was mentioned in original issue if you'd like to take a look!