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.

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

Investigate SAG/SAGA solver #23180

Open
lorentzenchr opened this issue Apr 21, 2022 · 7 comments
Open

Investigate SAG/SAGA solver #23180

lorentzenchr opened this issue Apr 21, 2022 · 7 comments
Labels
Bug help wanted Moderate Anything that requires some knowledge of conventions and best practices module:linear_model

Comments

@lorentzenchr
Copy link
Member

lorentzenchr commented Apr 21, 2022 •
edited
Loading

Description

The newly introduced tight tests for Ridge in #22910 together with the random seed fixture in #22749 revealed some shortcomings of the sag and saga solver:

  1. It shows some random behavior even with fixed random seed.
  2. The tol needs to be set much smaller to receive comparable results with the other solvers of Ridge.

Ideally, the cause for both issues can be identified and fixed.

Some links for context

#23014
#23017
#23026
#23152
#23177
@github-actions github-actions bot added the Needs Triage Issue requires triage label Apr 21, 2022
@lorentzenchr lorentzenchr added module:linear_model Bug and removed Needs Triage Issue requires triage labels Apr 21, 2022
@jeremiedbb
Copy link
Member

jeremiedbb commented Apr 21, 2022 •
edited
Loading

It shows some random behavior even with fixed random seed.

Actually the randomness will probably be fixed by #23181
It was probably not randomness from the solver but from the CI runs. The fact that when it failed it was always with sag and saga still shows that something is not going as expected.

@ogrisel
Copy link
Member

ogrisel commented May 9, 2022

If it can reach the same results with a small values of tol, it does not seem like a bug to me.

Are there any example where the solvers do not converge to the same solution even with a small tol?

@lorentzenchr
Copy link
Member Author

The point is that sag/saga often needs orders of magnitude smaller tol. In test_ridge.py it's 1e-15 vs 1e-10. We could (and do) live with that. Maybe there is room to improve on that.

@ogrisel
Copy link
Member

ogrisel commented Oct 14, 2022

Coming back to this issue, I think it would be interesting to find a way to normalize the tol criterion between all ridge solvers including sag/saga so that they approximately mean the same thing.

@ogrisel ogrisel added Moderate Anything that requires some knowledge of conventions and best practices help wanted labels Oct 14, 2022
@glevv
Copy link
Contributor

glevv commented Oct 28, 2022

could this be connected to #24679 ?

@TomDLT
Copy link
Member

TomDLT commented Nov 22, 2022 •
edited
Loading

could this be connected to #24679 ?

No. The linked issue is a potential mistake on the prox operator in SAGA. However, the prox operator is not used in Ridge, because the prox operator is only used for L1 regularization. Therefore, this potential mistake should not affect Ridge convergence.

edit: #24679 was not a mistake.

@lorentzenchr
Copy link
Member Author

Maybe it's related to the scaling of the objective similar to #24752?

@glemaitre glemaitre moved this to Discussion in Losses and solvers May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug help wanted Moderate Anything that requires some knowledge of conventions and best practices module:linear_model
Projects
Losses and solvers
Status: Discussion
Milestone
No milestone
Development

No branches or pull requests
5 participants
@ogrisel @TomDLT @lorentzenchr @jeremiedbb @glevv