-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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.
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
Acquisition function API redesign #447
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #447 +/- ##
==========================================
+ Coverage 94.68% 96.23% +1.54%
==========================================
Files 9 10 +1
Lines 677 849 +172
==========================================
+ Hits 641 817 +176
+ Misses 36 32 -4 ☔ View full report in Codecov by Sentry. |
Some more changes:
@fmfn @bwheelz36 @leandrobbraga I will wait with merging this until the docstrings are finished, but if you'd like to give some feedback already, please do. |
Wow this is a huge amount of work. Impressive! Before we start review can you resolve the merge conflicts and make sure all the tests pass? |
Will do 👍 ideally I want to wait until the docs are finished to only deal with one conflict. After that I will mark the PR as ready to review :) |
@till-m - will be a breaking redesign? Not necessarily against it, just want to be clear. |
There will be some necessary breaking changes, primarily to workflows that change the acquisition function -- this is unavoidable, since the old If someone just calls |
@bwheelz36 I know this is a lot. I'm sorry about that 😬 |
return x_max | ||
|
||
|
||
class GPHedge(AcquisitionFunction): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want to go overboard and add too many acquisition functions. GPHedge
and the ConstantLiar
seemed appropriate. EIpu (#393) is additionally showcased in the notebook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- great work @till-m. I have very few comments. I agree that this seems cleaner and more maintainable. I haven't used many different acquisition functions, so I don't have any comments on the specific implementations.
- nitpicking, there's a number of lines
if self.y_max is None:
which are not covered by a test case. but coverage is >95% which is already extremely high, so if you don't think these need to be covered I'm fine with it. - It is a breaking change, so when we release the next version it should be
2.0.0
- Given the significance of the change, ideally we'd get a review from multiple people before merging. But if no one else chimes in after a week or so I think you should merge.
Coverage is fixed now! :) Agree on all other points. Thanks for the review! :) |
Redesign the way acquisition functions work.
Content:
UtilityFunction
class and replaces it with a genericAcquisitionFunction
base classacq_max
, let each acquisition function handle the maximization itselfUpperConfidenceBound
,ProbabilityOfImprovement
andExpectedImprovement
as possible utility functionsGPHedge
to find which of (a combination of) UCB/PoI/EI are best suited for a problem Implementgp_hedge
acquisition function #439Adds wrapper acquisition functionKrigingBeliever
to allow for async optimization Proper parallel optimisation #347 feat(bayesian_optimization.py): Allow registering of temporary observations #365ConstantLiar
to allow for async optimization Proper parallel optimisation #347 feat(bayesian_optimization.py): Allow registering of temporary observations #365. Strategies aremin
,max
,mean
andconstant
(latter chosen by supplying a number).Sets default unconstrained acquisition function to GPHedge with base functionsSets default unconstrained acquisition function toUCB(kappa=2.576)
,PoI(xi=0.01)
,EI(xi=0.01)
.UCB(kappa=2.576)
Set GPHedge with base functionsSets default constrained acquisition function toPoI(xi=0.01)
,EI(xi=0.01)
as standard acquisition function for constrained improvementEI(xi=0.01)
suggest
,maximize
etc. don't take an acquisition function argument anymore.Why?
The old
UtilityFunction
andacq_max
were the most inflexible part of this package and their design was somewhat convoluted (e.g. providing axi
parameter when using UCB). Now it should be much easier to add custom acquisition functions.