10000 [WIP] Change default convergence params for classes using liblinear (LinearSVM and LogisticRegression) by summer-bebop · Pull Request #13317 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

[WIP] Change default convergence params for classes using liblinear (LinearSVM and LogisticRegression) #13317

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
Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

[WIP] Change default convergence params for classes using liblinear (LinearSVM and LogisticRegression) #13317

wants to merge 3 commits into from

Conversation

summer-bebop
Copy link
Contributor
@summer-bebop summer-bebop commented Feb 27, 2019 •
edited
Loading

Reference Issues/PRs

Fixes #11536

What does this implement/fix? Explain your changes.

This Pull request is meant to implement an automatic determination of the value of convergence parameters (max_iter and tol).

TODO

  • Initialize the structure of the code
  • Fill in the blank left in the code structure to implement the switch on the solver type to determine max_iter and tol
  • Set defaults corresponding to solver defaults
  • Check if solver defaults generates convergence warnings
  • Identify better defaults through benchmarks
  • Investigate changing the type of tol of lbfgs to ftol rather than pgtol

@summer-bebop summer-bebop changed the title [WIP] Change default convergence params for classes using liblinear (LinearSV{C,R} and LogisticRegression) [WIP] Change default convergence params for classes using liblinear (LinearSVM and LogisticRegression) Feb 27, 2019
@summer-bebop
Copy link
Contributor Author
summer-bebop commented Feb 28, 2019 •
edited
Loading

@GaelVaroquaux @agramfort All right after several checks I noticed that all solver libraries already have their own default on max_iter and tol (I do have a doubt about the default of max_iter in liblinear though). Hence I suggest the following strategie :

  • Default the max_iter and the tol to 'auto' in LinearRegression and LinearSVM
  • Implement a set_params called somewhere along the way of the function cascade to the actual solver call. This set_param would exclude max_iter and tol from the parameters provided to the solver if tol and max_iter equal 'auto' but would integrate max_iter and tol if they are provided by the user and pass their value along for the solver to use it.
  • Do some benchmark to check if the default behaviour of the solver generate convergence warnings
  • Adapt some default if that's the case

What do you think ? -> just implement a function called in the .fit that maps solvers to tol and max_iter

On another subject I see that besides lbfgs, the tol for liblinear, newton-cg are also related to the value of the gradient of the function and not to value of the function. So while I can change that for lbfgs as scipy also expose an ftol, newton-cg and liblinear don't. There is probably some room for improvement there ?

@summer-bebop
Update of the function with a dictionnay of iter and tol depending on…
1ed005c 
… the solver. Todo : branching detail for liblinear to select the internal solver, tests for auto value in max_iter and tol for logisticreg and linearsvm, benchmarks to check if solvers default are good, change lbfgs tol for an ftol instead of a pgtol
@summer-bebop
Liblinear branching done, tests done (maybe not exhaustives). Todo : …
40ed0aa 
…benchmarks to check if solvers defaults are good, change lbfgs tol for an ftol instead of a pgtol
@jnothman jnothman added this to the 0.21 milestone Apr 16, 2019
@jnothman jnothman modified the milestones: 0.21, 0.22 Apr 24, 2019
@jnothman jnothman mentioned this pull request Jul 14, 2019
Open
@amueller amueller added the Needs Decision Requires decision label Aug 6, 2019
@amueller
Copy link
Member
amueller commented Aug 6, 2019

What's the status on this?

@j 860E nothman jnothman modified the milestones: 0.22, 0.23 Oct 31, 2019
@adrinjalali adrinjalali removed this from the 0.23 milestone Apr 20, 2020
Base automatically changed from master to main January 22, 2021 10:50
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
module:linear_model module:svm module:utils Needs Decision Requires decision
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Liblinear convergence failure everywhere?
4 participants
@summer-bebop @amueller @jnothman @adrinjalali
0