Ask tell API #346
Ask tell API #346
Conversation
|
After discussion with @vpicheny , I am going to add a notebook to this PR, to help reviewers see the API in action. |
trieste/ask_tell_optimization.py
Outdated
|
|
||
| @classmethod | ||
| def from_record( | ||
| cls, search_space: SP, acquisition_rule: AcquisitionRule[S, SP], record: Record[S] |
There was a problem hiding this comment.
Maybe put record first and make acquisition_rule optional (assuming you end up doing that for init)? Also it might make sense to expose search_space and acquisition_rule as @properties. (There'd probably be no harm doing that for BayesianOptimizer too, though it's more obviously useful here.)
There was a problem hiding this comment.
yup, now that the rule can be optional this needs some reordering indeed
There was a problem hiding this comment.
what would properties be good for?
henrymoss
requested review from
uri-granta,
henrymoss,
vpicheny and
hstojic
|
|
||
| :return: An optimization state record. | ||
| """ | ||
| models_copy = copy.deepcopy(self._models) |
There was a problem hiding this comment.
should we check here if deepcopy method is available and give a user-friendly message that the model cannot be saved? though perhaps a better place for this check might be in models interfaces...
(Keras models don't have deepcopy I think, and would produce a less user-friendly message)
docstring could perhaps include a warning to users related to this, along the lines what you have in the notebook
There was a problem hiding this comment.
How can this be checked? Just because an object doesn't override __deepcopy__ doesn't mean it cannot be deepcopied. What would happen if one tries to deepcopy a Keras model?
There was a problem hiding this comment.
We could catch any exception and reraise with an additional error message, but I don't think that'll really add much value. Maybe mention in the docstring that this will fail if the models or acquisition state are not deep copyable?
|
|
||
| :return: An optimization state record. | ||
| """ | ||
| models_copy = copy.deepcopy(self._models) |
There was a problem hiding this comment.
We could catch any exception and reraise with an additional error message, but I don't think that'll really add much value. Maybe mention in the docstring that this will fail if the models or acquisition state are not deep copyable?
Related issue: #215
This PR adds high level ask/tell interface. It is essentially a stripped down
optimizefunction with the loop, all history and state tracking removed.Contrary to the discussion in the issue, I've went with simpler approach and no coroutines. This allows for a nice way of saving and loading state, which can be important in complex scenarios where the optimization has to be stopped for a considerable time to perform the experiment. There are also a few one line fixes in here.
Once we agree on the API, the next PR with docs and example notebook will follow.