Refactor recommender#826
Conversation
|
Stefan Schmid seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
AVHopp
left a comment
AVHopp
left a comment
There was a problem hiding this comment.
I like the general approach and direction. I left two minor comments related to typing issues, which are hopefully addressing the current mypy complaints. It would be great if you could have a look at those mypy complaints and try to fix them on your own already now, as they hint and some minor design problems. The other questions you raised can be discussed later then once this is fixed :)
| ) | ||
| """The acquisition function. When omitted, a default is used.""" | ||
|
|
||
| optimizer: OptimizerProtocol | None = field( |
There was a problem hiding this comment.
I think allowing None here is currently causing typing problems. I think the patter should actually just be optimizer: OptimizerProtocol which is then automatically being set in the derived classes. That is, the BotorchRecommender would set the default direclty without the check for whether or not this is None, and at some point, we might be able to give a reasonable default working for all kind of search spaces already here.
There was a problem hiding this comment.
Was going in the same direction but more critically so:
- Strictly speaking, this new argument is already a breaking change (but not saying we don't need it, just needs more refinement)
- Agree with @AVHopp:
Nonedoesn't make sense – we always need an optimization procedure. Without it, the recommender is non-functional - This refactoring in fact opens up a larger debate: potentially, we can kick the
BotorchRecommenderaltogether, and instead use theBayesianRecommenderas a non-abstract class in the future. Reason: so far, theBotorchRecommenderencapsulated the specific botorch routines in its body. With the refactoring, this is no longer the case and the part is modularized. That is, we move from inheritance to composition --> aBayesianRecommendertakes over the role of the previousBotorchRecommenderwhen it is constructed with the corresponding botorch routines as arguments.
| optimization is applied automatically. | ||
| """ | ||
|
|
||
| def __call__( |
There was a problem hiding this comment.
I think you are missing the @override annotation
AdrianSosic
left a comment
AdrianSosic
left a comment
There was a problem hiding this comment.
First steps done 👏🏼 but here some comments regarding the design
| ) | ||
| """The acquisition function. When omitted, a default is used.""" | ||
|
|
||
| optimizer: OptimizerProtocol | None = field( |
There was a problem hiding this comment.
Was going in the same direction but more critically so:
- Strictly speaking, this new argument is already a breaking change (but not saying we don't need it, just needs more refinement)
- Agree with @AVHopp:
Nonedoesn't make sense – we always need an optimization procedure. Without it, the recommender is non-functional - This refactoring in fact opens up a larger debate: potentially, we can kick the
BotorchRecommenderaltogether, and instead use theBayesianRecommenderas a non-abstract class in the future. Reason: so far, theBotorchRecommenderencapsulated the specific botorch routines in its body. With the refactoring, this is no longer the case and the part is modularized. That is, we move from inheritance to composition --> aBayesianRecommendertakes over the role of the previousBotorchRecommenderwhen it is constructed with the corresponding botorch routines as arguments.
| # See also: https://www.attrs.org/en/stable/glossary.html#term-slotted-classes | ||
| __slots__ = () | ||
|
|
||
| def __call__( |
There was a problem hiding this comment.
This interface is the critical part. Right now, it doesn't look wrong and seems to serve the purpose. Let's closely monitor this while developing the PR
| def __call__( | ||
| self, | ||
| batch_size: int, | ||
| acquisition_function: BoAcquisitionFunction, |
There was a problem hiding this comment.
In contrast to the Optimizer protocol, this one does not work:
- First: you are actually violating the base protocol! Base uses baybe-type acqf, but here you use botorch-type acqf.
- If it really turns out that the latter is needed, this points at a code design problem: your interface sits in between two layers of abstractions, i.e. it uses both botorch-types and baybe-types, which is usually a code smell. It's like having an interface that ingests pandas-dataframes, numpy-arrays and torch-tensors at the same time
4 participants
Hi @AVHopp and @AdrianSosic
as discussed yesterday, here is a very rough first draft of how a possible outline for the recommender refactoring could look like, on the example of the gradient based optimizer for continuous search spaces. Meant more as a discussion starter, not as a PR for stringent review :)
In short, what I tried to do was to create a new protocol for optimizers, and then start with a low-level optimizer as a demonstrative example. Some open questions where I would appreciate your thoughts:
n_restarts) is still left as an argument to the recommender (due to backwards compatibility), and then handed down to the optimizer. Not completely sure how to handle the compatibility optimally.Thanks both!