New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MRG+2] Adding Implementation of SAG - next episode #4738

Closed
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@TomDLT
Member

TomDLT commented May 19, 2015

I took over the great work of @dsullivan7 in #3814.

I removed the merges with master, squashed all the commits and rebased on master.

@dsullivan7

This comment has been minimized.

Contributor

dsullivan7 commented May 19, 2015

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

@amueller

This comment has been minimized.

Member

amueller commented May 19, 2015

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

@TomDLT

This comment has been minimized.

Member

TomDLT commented May 19, 2015

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):
diffloss
Result on RCV1 (804.414 x 47.152, Sparse):
diffloss

@agramfort

This comment has been minimized.

Member

agramfort commented May 19, 2015

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

@amueller

This comment has been minimized.

Member

amueller commented May 19, 2015

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.

@amueller

This comment has been minimized.

Member

amueller commented May 19, 2015

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

@TomDLT

This comment has been minimized.

Member

TomDLT commented May 19, 2015

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.
diffloss_no_intercept

@amueller

This comment has been minimized.

Member

amueller commented May 19, 2015

Thanks for the explanation.

@TomDLT

This comment has been minimized.

Member

TomDLT commented May 20, 2015

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)
@amueller

This comment has been minimized.

Member

amueller commented May 20, 2015

why do we lose warm_start?

@agramfort

This comment has been minimized.

Member

agramfort commented May 21, 2015

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)

@agramfort

This comment has been minimized.

Member

agramfort commented May 21, 2015

travis is not happy

@TomDLT

This comment has been minimized.

Member

TomDLT commented May 21, 2015

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

@agramfort

This comment has been minimized.

Member

agramfort commented May 21, 2015

@agramfort

This comment has been minimized.

Member

agramfort commented May 21, 2015

ping us when ready to merge.

thx

@amueller

This comment has been minimized.

Member

amueller commented May 21, 2015

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

@amueller

This comment has been minimized.

Member

amueller commented May 21, 2015

needs a rebase (probably for whatsnew)

@TomDLT

This comment has been minimized.

Member

TomDLT commented May 21, 2015

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
@agramfort

This comment has been minimized.

Member

agramfort commented May 21, 2015

@amueller

This comment has been minimized.

Member

amueller commented May 21, 2015

yeah

@amueller

This comment has been minimized.

Member

amueller commented May 21, 2015

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

@agramfort

This comment has been minimized.

Member

agramfort commented May 21, 2015

@TomDLT

This comment has been minimized.

Member

TomDLT commented May 21, 2015

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.

@agramfort

This comment has been minimized.

Member

agramfort commented May 21, 2015

@TomDLT

This comment has been minimized.

Member

TomDLT commented May 22, 2015

So I added random_state in sag_ridge.

@TomDLT

This comment has been minimized.

Member

TomDLT commented May 22, 2015

By the way, _fit_liblinear has a random_state parameter, but it is not used in LogisticRegression with solver='liblinear'.
Actually, LogisticRegression has a random_state parameter that was never used (before SAG)

@agramfort

This comment has been minimized.

Member

agramfort commented May 22, 2015

@TomDLT

This comment has been minimized.

Member

TomDLT commented May 27, 2015

Here is the implementation of SAG as 2 solvers (sag_ridge and sag_logistic).
I also updated the doc and the tests.

I checked speed performances, compared to previous class implementation(SAGClassifier):
Classifier for RCV1 dataset:
classifier_rcv1_dloss
Regressor for Alpha dataset:
regressor_alpha_dloss_nointercept_alone

@TomDLT

This comment has been minimized.

Member

TomDLT commented May 27, 2015

Please tell me what you think of the code.

Also, do we want to change the API for Ridge and LogisticRegression, in order to add a warm_start parameter?

@agramfort

This comment has been minimized.

Member

agramfort commented Sep 9, 2015

@@ -156,6 +158,12 @@ Enhancements
visible with extra trees and on datasets with categorical or sparse
features. By `Arnaud Joly`_.
- Added optional parameter ``random_state`` in :class:`linear_model.Ridge`
, to set the seed of the pseudo random generator used in ``sag`` solver.By `Tom Dupre la Tour`_.

This comment has been minimized.

@amueller

amueller Sep 9, 2015

Member

space before "By" ;)

@amueller

This comment has been minimized.

Member

amueller commented Sep 9, 2015

@ogrisel we want this in the release, right?

@amueller

This comment has been minimized.

Member

amueller commented Sep 9, 2015

Great work everybody :)

@amueller amueller added this to the 0.17 milestone Sep 9, 2015

@ogrisel

This comment has been minimized.

Member

ogrisel commented Sep 9, 2015

@ogrisel we want this in the release, right?

I am not opposed to have it in :)

@ogrisel

This comment has been minimized.

Member

ogrisel commented Sep 10, 2015

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.

@ogrisel

This comment has been minimized.

Member

ogrisel commented Sep 10, 2015

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

@amueller

This comment has been minimized.

Member

amueller commented Sep 10, 2015

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

@amueller

This comment has been minimized.

Member

amueller commented Sep 10, 2015

Pushed as 94eb619. Thanks for the great work!

@amueller amueller closed this Sep 10, 2015

@agramfort

This comment has been minimized.

Member

agramfort commented Sep 10, 2015

@ogrisel

This comment has been minimized.

Member

ogrisel commented Sep 11, 2015

🍻!

@dsullivan7

This comment has been minimized.

Contributor

dsullivan7 commented Sep 11, 2015

Awesome!!

@TomDLT

This comment has been minimized.

Member

TomDLT commented Sep 11, 2015

Nice !

@fabianp

This comment has been minimized.

Member

fabianp commented Sep 11, 2015

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

@TomDLT

This comment has been minimized.

Member

TomDLT commented Sep 11, 2015

Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment