Skip to content

Use common convergence checks for lbfgs solver#14250

Merged
agramfort merged 7 commits intoscikit-learn:masterfrom
rth:lbfgs-convergence-helper
Jul 12, 2019
Merged

Use common convergence checks for lbfgs solver#14250
agramfort merged 7 commits intoscikit-learn:masterfrom
rth:lbfgs-convergence-helper

Conversation

@rth
Copy link
Copy Markdown
Member

@rth rth commented Jul 3, 2019 •
edited
Loading

First step toward #14248

This replaces to calls to fmin_l_bfgs_b with scipy.optimize.minimize(method="L-BFGS-B") and uses a common helper to check for convergence (for instance previously no warnings were shown in Gaussian process models). It also consistently applies the fix for max_iter,

   	 # In scipy <= 1.0.0, nit may exceed maxiter.
     # See https://github.com/scipy/scipy/issues/7854.
     self.n_iter_ = min(dict_['nit'], self.max_iter)

that was not done in neural_network module. Follow up on #9274 (review).

Although this PR is mostly an internal refactoring, it also fixes the above mentioned issues, and could be considered a bug fix.

The message in case of a ConvergenceWarning is changed, so in that sense its somewhat non backward compatible change, but because the warning type is very specific I don't know how much that matters.

Note: fmin_l_bfgs_b is now marked as a legacy function in scipy,

The functions below are not recommended for use in new scripts; all of these methods are accessible via a newer, more consistent interfaces, provided by the interfaces above.

The mapping between legacy and new parameters for the optimization result can be found here

@rth rth changed the title Use common convergence checks for lbfgs solver WIP Use common convergence checks for lbfgs solver Jul 3, 2019
@rth rth changed the title WIP Use common convergence checks for lbfgs solver Use common convergence checks for lbfgs solver Jul 3, 2019
@adrinjalali adrinjalali self-assigned this Jul 8, 2019
adrinjalali
adrinjalali reviewed Jul 8, 2019
View reviewed changes
sklearn/gaussian_process/gpc.py
Per default, the 'L-BFGS-B' algorithm from scipy.optimize.maximize
is used. If None is passed, the kernel's parameters are kept fixed.
Available internal optimizers are::

Copy link
Copy Markdown
Member

@adrinjalali adrinjalali Jul 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we then deprecate fmin_l_bfgs_b as the input value in favor of L-BFGS-B, or lbfgs?

Copy link
Copy Markdown
Member Author

@rth rth Jul 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think that would be in order, and similarly I wanted to see if allowing other scipy optimizers in Gaussian processes would be interesting. Though I would rather do that in a follow-up PR, and keep this as a minimal refactoring not affecting backward compatibility.

Copy link
Copy Markdown
Member

@adrinjalali adrinjalali Jul 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree allowing other optimizers would be a different PR, but since you're touching the docstring here, it makes sense for the accepted value to be the same or similar to what you mention in the docstring, I think.

But if you wanna do the deprecation in a different PR, I'm happy with that as well, and then this LGTM.

Copy link
Copy Markdown
Member Author

@rth rth Jul 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't changed the docstring value here only changed that the algorithm is called L-BFGS-B, not fmin_l_bfgs_b. That fix would apply even before this PR as fmin_l_bfgs_b is not an algorithm name it's the scipy function name for that optimizer.

Will do the deprecated in a follow up PR :) Thanks for the review!

adrinjalali
adrinjalali approved these changes Jul 8, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rth, I really like PRs like this one.

@rth rth mentioned this pull request Jul 9, 2019
Merged
7 tasks
agramfort
agramfort approved these changes Jul 12, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great!

@agramfort agramfort merged commit 56a0ed1 into scikit-learn:master Jul 12, 2019
@agramfort
Copy link
Copy Markdown
Member

agramfort commented Jul 12, 2019

thx heaps @rth

@rth rth deleted the lbfgs-convergence-helper branch July 12, 2019 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@agramfort agramfort agramfort approved these changes

@adrinjalali adrinjalali adrinjalali approved these changes

Assignees

@adrinjalali adrinjalali

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rth @agramfort @adrinjalali