8000 [MRG+2] Adding Implementation of SAG - next episode by TomDLT · Pull Request #4738 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 11 commits into from
Closed

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

wants to merge 11 commits into from

Conversation

TomDLT
Copy link
Member
@TomDLT 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
Copy link
Contributor

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

@amueller
Copy link
Member

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

@TomDLT
Copy link
Member Author
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
Copy link
Member

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

@amueller
Copy link
Member

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
Copy link
Member

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

@TomDLT
Copy link
Member Author
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
Copy link
Member

Thanks for the explanation.

@TomDLT
Copy link
Member Author
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
Copy link
Member

why do we lose warm_start?

@agramfort
Copy link
Member

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
Copy link
Member
8000

travis is not happy

@TomDLT
Copy link
Member Author
TomDLT commented May 21, 2015

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

@agramfort
Copy link
Member
agramfort commented May 21, 2015 via email

@TomDLT TomDLT force-pushed the sag branch 2 times, most recently from 38af560 to 2e9618b Compare May 21, 2015 09:06
@agramfort
Copy link
Member

ping us when ready to merge.

thx

@amueller
Copy link
Member

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

@amueller
Copy link
Member

needs a rebase (probably for whatsnew)

@TomDLT
Copy link
Member Author
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

@TomDLT TomDLT force-pushed the sag branch 4 times, most recently from b37cfcc to a1adf82 Compare May 21, 2015 16:35
@agramfort
Copy link
Member
agramfort commented May 21, 2015 via email

@amueller
Copy link
Member

yeah

@amueller
Copy link
Member

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

@agramfort
Copy link
Member
agramfort commented May 21, 2015 via email

@TomDLT
Copy link
Member Author
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.

@amueller
Copy link
Member
amueller commented Sep 9, 2015

Great work everybody :)

@amueller amueller added this to the 0.17 milestone Sep 9, 2015
@ogrisel
Copy link
Member
ogrisel commented Sep 9, 2015

@ogrisel we want this in the release, right?

I am not opposed to have it in :)

@ogrisel
Copy link
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
Copy link
Member
ogrisel commented Sep 10, 2015

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

@amueller
Copy link
Member

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

@amueller
Copy link
Member

Pushed as 94eb619. Thanks for the great work!

@amueller amueller closed this Sep 10, 2015
@agramfort
Copy link
Member
agramfort commented Sep 10, 2015 via email

@ogrisel
Copy link
Member
ogrisel commented Sep 11, 2015

🍻!

@dsullivan7
Copy link
Contributor

Awesome!!

@TomDLT
Copy link
Member Author
TomDLT commented Sep 11, 2015

Nice !

@fabianp
Copy link
Member
fabianp commented Sep 11, 2015

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

@TomDLT
Copy link
Member Author
TomDLT commented Sep 11, 2015

Thanks :)

@TomDLT TomDLT deleted the sag branch October 16, 2015 12:07
@TomDLT TomDLT mentioned this pull request Jan 16, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.17
Development

Successfully merging this pull request may close these issues.
10 participants
@TomDLT @dsullivan7 @amueller @agramfort @GaelVaroquaux @StevenLOL @mblondel @vene @ogrisel @fabianp
0