Use common convergence checks for lbfgs solver #14250

rth · 2019-07-03T17:19:24Z

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

adrinjalali · 2019-07-08T09:18:11Z

sklearn/gaussian_process/gpc.py

@@ -74,7 +73,7 @@ def optimizer(obj_func, initial_theta, bounds):
                # the corresponding value of the target function.
                return theta_opt, func_min

-        Per default, the 'fmin_l_bfgs_b' algorithm from scipy.optimize
+        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::



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

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.

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.

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

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

agramfort

looks great!

agramfort · 2019-07-12T11:34:28Z

thx heaps @rth

rth added 5 commits July 3, 2019 17:35

Use minimize for lbfgs in logistic regression

aa47a11

Use minimize for lbfgs in huber

3538017

Adapt gpr and gpc

10b7704

Refactor neural_network

13f2b15

More fixes

e94627e

rth changed the title ~~Use common convergence checks for lbfgs solver~~ WIP Use common convergence checks for lbfgs solver Jul 3, 2019

Fix tolerance argument

c716f3f

rth changed the title ~~WIP Use common convergence checks for lbfgs solver~~ Use common convergence checks for lbfgs solver Jul 3, 2019

adrinjalali self-assigned this Jul 8, 2019

adrinjalali reviewed Jul 8, 2019
View reviewed changes

Fix maximize -> minimize in docstring

4ac0686

adrinjalali approved these changes Jul 8, 2019

View reviewed changes

rth mentioned this pull request Jul 9, 2019

Minimal Generalized linear models implementation (L2 + lbfgs) #14300

Merged

7 tasks

agramfort approved these changes Jul 12, 2019

View reviewed changes

agramfort merged commit 56a0ed1 into scikit-learn:master Jul 12, 2019

rth deleted the lbfgs-convergence-helper branch July 12, 2019 12:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use common convergence checks for lbfgs solver #14250

Use common convergence checks for lbfgs solver #14250

Use common convergence checks for lbfgs solver #14250

Use common convergence checks for lbfgs solver #14250

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment