Skip to content
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

[DEV] Adding julia solvers: GD, SGD, SAGA #1

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ngazagna
Copy link

No description provided.

@agramfort
Copy link
Contributor

@ngazagna can you show here the figures you obtain?

@tomMoral
Copy link
Member

Thanks a lot @ngazagna for moving the PR! 😃 I will take a look ASAP.

It seems that the tests are not run in PRs, we also need to fix the CI.

@ngazagna
Copy link
Author

Sorry, I just had time to open the PR without testing it. Please find hereafter the plots obtained after running

benchopt run ./benchmark_logreg_l2

julia_solvers_logreg_l2_fig1
julia_solvers_logreg_l2_fig2

@agramfort
Copy link
Contributor

agramfort commented Sep 30, 2020 via email

@ngazagna
Copy link
Author

ngazagna commented Sep 30, 2020

There is indeed a problem with sklearn[lbfgs]. I think it is due to the lack of homogeneity in the tolerances used in sklearn solvers.
When passing solver='lbfgs to an sklearn LogisticRegression model, it runs this scipy optimize.minimize(method=’L-BFGS-B’) function, with gtol=tol which stops when

max{|proj g_i | i = 1, ..., n} <= gtol

where pg_i is the i-th component of the projected gradient.
Whereas, for instance with solver=newton-cg, then stops when

max{|g_i | i = 1, ..., n} <= tol

where g_i is the i-th component of the gradient.

EDIT: I guess this post is relevant

@agramfort
Copy link
Contributor

agramfort commented Sep 30, 2020 via email

@tomMoral
Copy link
Member

tomMoral commented Sep 30, 2020

could you push a ci trigger commit to make the test run on the PR?

git commit --allow-empty -m 'CI trigger'
git push

I guess alex is referring to the second plot where GD/SGd/SAGA seem to plateau to a different value.

@tomMoral
Copy link
Member

tomMoral commented Oct 1, 2020

@ngazagna I just did an update to fix the failing test so don't forget to use git pull before doing more modifications.

I will try to run the bench and see if I can see why this seems to not converge toward the same value.

@tomMoral
Copy link
Member

tomMoral commented Oct 2, 2020

Relies on benchopt/benchopt#78 for julia dependencies and pre_install_hook functionalities.

@tomMoral
Copy link
Member

tomMoral commented Oct 2, 2020

It seems there is no more technical bug :)

now, the failing test is because SGD does not find the optimal point (which is expected).
Maybe we should set some flag to detect stochastic solvers and skip this test?

@agramfort
Copy link
Contributor

agramfort commented Oct 2, 2020 via email

@tomMoral
Copy link
Member

tomMoral commented Oct 2, 2020 via email

@ngazagna
Copy link
Author

ngazagna commented Oct 2, 2020

could you push a ci trigger commit to make the test run on the PR?

git commit --allow-empty -m 'CI trigger'
git push

I guess alex is referring to the second plot where GD/SGd/SAGA seem to plateau to a different value.

Sorry, I did not get that Alex was pointing an issue related to SGD/SAGA. I guess we will face this problem as soon as algorithms need parameter tuning and when time in --timeout t is too small.

The other (expensive) option is do an extensive grid search (instead of the theoretical step size used here), try and retry until SGD converges.

@agramfort
Copy link
Contributor

agramfort commented Oct 2, 2020 via email

mathurinm pushed a commit to mathurinm/benchmark_logreg_l2 that referenced this pull request Nov 24, 2021
mathurinm pushed a commit to mathurinm/benchmark_logreg_l2 that referenced this pull request May 18, 2022
ogrisel pushed a commit to ogrisel/benchmark_logreg_l2 that referenced this pull request Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants