-
Notifications
You must be signed in to change notification settings - Fork 61
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
Callback #73
Callback #73
Conversation
…control over the models between iterations. Slightly reworked MCMCAcquisition to support this scenario
Codecov Report
@@ Coverage Diff @@
## master #73 +/- ##
=========================================
+ Coverage 99.8% 99.8% +<.01%
=========================================
Files 17 17
Lines 1013 1040 +27
=========================================
+ Hits 1011 1038 +27
Misses 2 2
Continue to review full report at Codecov.
|
self.counter = 0 | ||
|
||
def __call__(self, models): | ||
self.counter += 1 |
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.
Lets think about the callback signature some more. Is there any information we want to pass that might be useful for model building?
For instance, to let the model building strategy depend on the iteration number (we can stop optimizing the hyps after a while like in the MES paper). Although we can also look at the data set size.
What about model building strategies that changes model.X en model.Y (like replace clusters etc.). Not sure if that fits here or is even relevant (the GPflow model should be able to cope with it).
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 think the model contains all the data you need to accomplish something. I believe X and Y can even be updated in this callback as long as the model supports it (all models in GPflow do).
If at some point some information is really missing, this can be added.
GPflowOpt/acquisition/acquisition.py
Outdated
# the call to the constructor of the parent classes, will optimize acquisition, so it obtains the MLE solution. | ||
super(MCMCAcquistion, self).__init__([acquisition] + copies) | ||
super(MCMCAcquistion, self).__init__([acquisition]*n_slices) |
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.
Does this make deep copies? I assumed you used the old way to assure that it were deep copies
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.
Ah I see, need_new_copies = True makes sure deep copies are made later
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.
This version does shallow copies, its mostly to assure the copy later on is aware of the amount of copies required without serious overhead.
GPflowOpt/acquisition/acquisition.py
Outdated
self._sample_opt = kwargs | ||
|
||
def _optimize_models(self): | ||
# Optimize model #1 | ||
self.operands[0]._optimize_models() | ||
|
||
# Copy it again if needed due to changed free state | ||
if self._needs_new_copies: | ||
new_copies = [copy.deepcopy(self.operands[0]) for _ in range(len(self.operands) - 1)] |
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.
copy.deepcopy([self.operands[0]]*len(self.operands))
not tested, works too?
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.
no, the * syntax are shallow copies so the deepcopy will copy the object they are all pointing to.
GPflowOpt/acquisition/acquisition.py
Outdated
|
||
def _kill_autoflow(self): | ||
""" | ||
Following the recompilation of models, the free state might have changed. This means updating the samples can |
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.
"""
Flag for recreation on next optimize.
Following the ...
"""
GPflowOpt/acquisition/acquisition.py
Outdated
cause inconsistencies and errors. Flag for recreation on next optimize | ||
""" | ||
super(MCMCAcquistion, self)._kill_autoflow() | ||
self._needs_new_copies = True |
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 assume we cant use needs_setup for this?
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.
_needs_setup is triggered by a simple set_data
. This doesn't require new copies, only in case a callback changes the models (this should happen)
GPflowOpt/bo.py
Outdated
def jitchol_callback(models): | ||
""" | ||
Default callback for BayesianOptimizer. For all GPR models, increase the likelihood variance in case of cholesky | ||
faillures. This is similar to the use of jitchol in GPy |
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.
failures
"""
Increase the likelihood ...
This is similar to ... Default callback for BayesianOptimizers. Only usable with GPR models.
"""
GPflowOpt/bo.py
Outdated
from .pareto import non_dominated_sort | ||
|
||
|
||
def jitchol_callback(models): |
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.
callbacks can be in a separate callbacks.py file?
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 do not plan on shipping any additional callbacks (I might even get rid of this one, it got comitted by accident but it might improve stability?) so that file would be quite empty.
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.
Ok, I'm not in favor of including jitchol. I think there are other ways users can improve stability. First and foremost putting priors and transforms on the hyps.
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.
Given #74 I think we should really consider this. For standard scenario's with GPRs (which is what most people will start with) I think this might give an additional automated stability support (which can be disabled by setting the callback to None)
GPflowOpt/bo.py
Outdated
@@ -51,6 +74,12 @@ def __init__(self, domain, acquisition, optimizer=None, initial=None, scaling=Tr | |||
are obtained using Hamiltonian MC. | |||
(see `GPflow documentation <https://gpflow.readthedocs.io/en/latest//>`_ for details) for each model. | |||
The acquisition score is computed for each draw, and averaged. | |||
:param callable callback: (optional) this function or object will be called after each evaluate, after the | |||
data of all models has been updated with all models as retrieved by acquisition.models as argument without | |||
the wrapping model handling any scaling . This allows custom model optimization strategies to be implemented. |
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.
if we do a separate callbacks.py file some of the explanation can be moved there + module link
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.
see above
GPflowOpt/bo.py
Outdated
@@ -69,6 +98,8 @@ def __init__(self, domain, acquisition, optimizer=None, initial=None, scaling=Tr | |||
initial = initial or EmptyDesign(domain) | |||
self.set_initial(initial.generate()) | |||
|
|||
self._iter_callback = callback |
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.
why call it iter_callback and not model_callback?
GPflowOpt/bo.py
Outdated
@@ -86,6 +117,8 @@ def _update_model_data(self, newX, newY): | |||
assert self.acquisition.data[0].shape[1] == newX.shape[-1] | |||
assert self.acquisition.data[1].shape[1] == newY.shape[-1] | |||
assert newX.shape[0] == newY.shape[0] | |||
if newX.size == 0: |
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.
will this ever happen? As far as I know we cant empty GPflow models so data[0] will never be empty.
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.
this line avoids _needs_setup = True in case i.e. the EmptyDesign is configured as initial design (as is by default)
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.
As a sidenote, as GPflow doesn't support models with no data I actually see no use case for BOptimizer having an initial design parameter.
GPflowOpt/bo.py
Outdated
# If callback specified, and acquisition has the setup flag enabled (indicating an upcoming compilation, | ||
# run the callback. | ||
if self._iter_callback and self.acquisition._needs_setup: | ||
self._iter_callback([m.wrapped for m in self.acquisition.models]) |
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.
if there is no callback:
- setup is run and models are optimized on the first evaluate
with a callback: - models are optimized here but setup probably has not been run yet and needs_setup is still True -> models are optimized again on first evaluate? right?
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.
You confuse something here: you can optimize your model in the callback but this is one of the scenarios (which would require optimize_restarts to be 0 in order to avoid two optimizes). The primary use case is to only set the initial starting point.
(The reason the jitchol callback runs the optimization for a small number of steps is to check if no cholesky error occurs, not to optimize the model. )
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.
Ok, there was indeed some confusion here. I thought the callback would implement the complete model building strategy: setting hyps, running one or more optimizations, etc. This is still possible but you have to set optimize_restarts = 0
gpflowopt/acquisition/acquisition.py
Outdated
Flag for recreation on next optimize. | ||
|
||
Following the recompilation of models, the free state might have changed. This means updating the samples can | ||
cause inconsistencies and errors. Flag for recreation on next optimize |
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.
duplicate "Flag for recreation on next optimize"
gpflowopt/bo.py
Outdated
|
||
def jitchol_callback(models): | ||
""" | ||
Increase the likelihood in case of cholesky faillures. |
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.
failures
jitchol_callback(m.wrapped) # pragma: no cover | ||
|
||
if not isinstance(m, GPR): | ||
continue |
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.
maybe show a warning?
gpflowopt/bo.py
Outdated
@@ -190,6 +228,10 @@ def inverse_acquisition(x): | |||
|
|||
# Optimization loop | |||
for i in range(n_iter): | |||
# If callback specified, and acquisition has the setup flag enabled (indicating an upcoming compilation, |
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.
If a callback is specified,...
and close brackets :)
Implementation of the final use case defined in #7 . This implements a callback strategy to plug a user defined callable in to BayesianOptimizer which is called each iteration and gives full controls over the models. All GPflow manipulations are possible (assigning priors, modifying transforms, fixing parameters). Goal of those callbacks is to assure optimizations are successful, which can be very application specific.
Combined with the optimize_restarts feature, following use-cases are possible:
This PR depends on #68 and #72 and should be merged after.