Improve acq_max seeding of L-BFGS-B optimization

[ptapping]

The utils.acq_max() function attempts to find the maximum of the function space by first random sampling, then performing a L-BFGS-B optimization starting from some number of random points in the parameter space. This function is used by the optimizer.suggest() routine.

I was using some code like this to do a quick-and-dirty estimate of the current optimal parameters, which should simply find the maximum value of the currently fitted Gaussian process model function:

suggestion = optimizer.suggest(UtilityFunction(kind='ucb', kappa=0.0, xi=0.0))
print(f"Best guess: {suggestion} = {optimizer._gp.predict([[suggestion['x'], suggestion['y']]])[0]}")

This worked most of the time, but I noticed that the suggestion is sometimes worse than the current best result in optimizer.res. In other words, the returned "best" y value can actually be worse than the best known y_max which is passed in as a parameter!

It turns out while suggest() passes acq_max() the best y_max value it has found so far, it doesn't let it know what set of parameters were used to obtain it. This commit passes in these parameters to the acq_max() function where they are added to the list of seeds used for the L-BFGS-B optimization routine. This ensures that the returned best value is at least as good as y_max, but also means the optimization routine should be able to "walk up" the local maximum around the current best y_max.

The changes to acq_max() uses a new keyword parameter with a default of None which retains API backwards compatibility.

In addition, the best candidate from the random sampling phase is also added to the list of seeds for the L-BFGS-B optimization routine, allowing the algorithm to "polish" that result by walking up its local maximum as well.

[matheus695p]

TOP !

[bwheelz36]

@ptapping

• sorry about slow response; I'm trying to work through all the open issues and pull requests....
• could you please make any random commit (e.g. add a space or a comment) to this branch, which will trigger the (newly working) CI to run (actually - can you add the new parameter to the docstring?)
• suggest is also called from maximize - in your opinion should this behavior be coded into maximize as well? Probably right?

[ptapping]

Hi @bwheelz36

Good stuff. I'm on a bit of a holiday at the moment, but will take a look at this soon when I'm back in front of my real computer.

[ptapping]

Hi @bwheelz36 ,

Better late than never! I finally updated this pull request to sync with the changes in master over the last two years. My testing seems to indicate it all still works as expected.

The docstring is added to the acq_max() function.

I don't think any other changes are required inside of maximize() as the suggest() call takes care of everything transparently.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (11a0c6a) 98.42% compared to head (c03dee0) 98.43%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #297   +/-   ##
=======================================
  Coverage   98.42%   98.43%           
=======================================
  Files           8        8           
  Lines         573      576    +3     
  Branches       84       85    +1     
=======================================
+ Hits          564      567    +3     
  Misses          5        5           
  Partials        4        4           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[brendan-whelan-seetreat]

Looks good to me, @till-m unless you have any concerns I can merge?

[till-m]

LGTM :)