Awesome @TomDLT! There was talk of having this implemented as a solver for LogisticRegression and RidgeRegression, have you looked into that at all?

travis is still unhappy ;) Thanks for picking this up!

I rerun the classifier benchmark on two large datasets:
RCV1 and Alpha (cf here)
The plot shows the convergence with log10(|loss - loss_optimal|)

Result on Alpha (500.000 x 500, Dense):

Result on RCV1 (804.414 x 47.152, Sparse):

as discussed I vote for adding 'sag' solver to LogisticRegression and RidgeRegression that would call plain sag_logistic and sag_ridge functions.

newtoncg is faster than liblinear? I'm surprised! Anyhow SAG seems to kick ass. I'd be +1 on adding a solver to the classifiers as this seems like a good default.

(though i dream of the day where the default LogisticRegression is multinomial, not OvR ;)

newton-cg is faster than liblinear? I'm surprised!

Actually, newton-cg is not faster.
In previous example, with fit_intercept=True, liblinear and newton-cg does not converge to the same minimum, since liblinear use a regularization on the intercept, whereas newton-cg and SAG don't.

I tried using the same regularization in SAG, and it converges to the same minimum as liblinear.
However, it makes more sense not to regularize the intercept.

Finally, with fit_intercept=False, we see that liblinear is a not slower than newton-cg.

Thanks for the explanation.

I implemented sag_logistic as a solver in LogisticRegression, and changed accordingly some of the tests.

Currently, in order to match LogisticRegression API, and compared to previous SAGClassifier,

• eta is forced to 'auto'
• we loose the warm_start
• we loose parallel processing for multiclass
• the behavior with multiclass with class weights is changed (it is now the same as in other LogisticRegression solvers with 'OvR' strategy)

why do we lose warm_start?

there is no reason not to support warm_start

I would put all sag related code in 1 file called sag.py (ie no sag_class.py)

travis is not happy

For warm_start, how should I pass the option without adding parameters to LogisticRegression?

ping us when ready to merge.

thx

I also thought there was a warm_start for LogisticRegressionCV... hum

needs a rebase (probably for whatsnew)

I implemented sag_ridge as a solver in Ridge, and changed accordingly some of the tests.

Currently, in order to match Ridge API, and compared to previous SAGRegressor,

• eta is forced to 'auto'
• we loose random_state and warm_start

yeah

shouldn't LogisticRegression have a random_state for liblinear? Or is that only for hinge-loss?

No, LogisticRegression class has a random_state, and so has sag_logistic.
It is missing in Ridge class, so it is currently missing also in sag_ridge.

Thanks again for the review!

FYI I am working on a multinomial version of SAG, but it will be in another PR.

@ogrisel we want this in the release, right?

Great work everybody :)

@ogrisel we want this in the release, right?

I am not opposed to have it in :)

FYI I am working on a multinomial version of SAG, but it will be in another PR.

Would be great to consider adding support for sample_weight to LogisticRegression and LogisticRegressionCV as well while you are at it.

This PR needs a rebase on top of the current master.

I'll rebase, squash and merge in a bit unless anyone complains.

Pushed as 94eb619. Thanks for the great work!

🍻!

Awesome!!

Nice !

Yeah! @TomDLT deserves extra kudos for patience and perseverance :-)

Thanks :)