First draft Continuous optimizer

[StefanPSchmid]

Hey @AdrianSosic and @AVHopp

creating a new PR, this time on the baybe repo instead of my fork, so you can commit. Kept the previous changes with some slight modifications, and copy your comments here so we can have the discussions here. Code is changed only insignificantly (I know there are still mypy issues for example, I have some questions about your opinions first), I mainly have new thoughts/questions in the comments :)

[StefanPSchmid]

Comment from @AVHopp on other PR:
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.

Comment from @AdrianSosic on other PR:
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: None doesn'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 BotorchRecommender altogether, and instead use the BayesianRecommender as a non-abstract class in the future. Reason: so far, the BotorchRecommender encapsulated 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 --> a BayesianRecommender takes over the role of the previous BotorchRecommender when it is constructed with the corresponding botorch routines as arguments.

@StefanPSchmid's new thoughts/questions:

• I was orienting myself at the acquisition_function , which also can have None at this field and is only set later to a sensible default. I was thinking of doing the same here, i.e. in recommend there would be a new function setup_optimizer that sets a default (depending on the search space type) when the given optimizer is None. The problem from my POV currently is that I am still trying to make it work that when the user gives some arguments to BotorchRecommender for the Optimizer (e.g. sequential_continuous), that they are arguments of the Recommender, and not available at the setup stage (if I understood the code directly).
• I think for that matter too, I would be for removing the BotorchRecommender, that would move the dispatching based on searchspace type into BayesianRecommender and that gets then optimized with a specific Optimizer. Then there would also not be this dychtomy where these arguments should be. One thing of which I am very unsure is how this would be handled with backwards compatibility, currently trying to get it to work even when old interface of BotorchRecommender is called (see point above).

So to summarize - I would remove the BotorchRecommender, put the dispatching logic into BayesianRecommender and implement a _setup_acquisition_function style function for the optimizer too. What do you think?

[AdrianSosic]

I think dropping the BotorchRecommender is the right thing to do 👍🏼 At least from what I can currently overlook. But we need to ensure backward compatibility here, so we need some deprecation for it.

Regarding the None discussion: I think it really depends on what we expect from the Optimizer object. I think your current idea is that you want to dispatch between a conti/disc/hybrid optimizer depending on the given search space, in which case you'd indeed need a delayed default. However, the alternative is that we simply require the optimizer to be able to handle all three kinds of search spaces. While neither of the simple optimizers fulfills it, we'll later have composite optimizers that do so. For example, the alternating optimizer can handle all spaces (with purely conti/disc just being special cases where the sequence stops immediately after the first subspace). And due to the recursive interface, composite optimizers also fulfill the required type, meaning we could set an appropriate composite recommender as default. Similarly, all hybrid optimizers trivially also support purely conti/disc spaces.

But this doesn't need to be decided now, we can follow up on it later when all puzzle pieces are in place.

[AVHopp]

Note that I left some thoughts about this in the old PR, explaining a bit the potential role of None (which might however not be accurate now that I see this discussion here 😆)

[AVHopp]

@AdrianSosic can you elaborate on why this is a breaking change already? Is it only because it is public? Because right now, it can still simply be ignored, right?

Let me see if I get @AdrianSosic's idea correctly and whether we can agree on it:

• There are two kinds of optimizers: PureOptimizer which handle exactly one kind of space (probably not user facing) and CompositeOptimizers which combine PureOptimizers (or other CompositeOptimizers?) to handle all kinds of search spaces
• We could then always have a CompositeOptimizer as default which knows how to handle all sorts of search spaces
• If an optimization procedure is only intended to be used for e.g. discrete search spaces, then this is implemented as PureOptimizer. However, this will then be (automatically) be wrapped into a CompositeOptimizer

I have to say that I am not sure what I think of this idea. While I like it from a code design perspective, it makes me wonder what other implications this would have. For example, wouldn't we then somehow implicitly assume that every search space is hybrid if our recommendation procedure always needs to know how to handle hybrid spaces? Also, for a user wanting to only do some optimization in a discrete space, it might be weird if things are being wrapped into some other object that could then also be used for other spaces.

Currently, I'd thus prefer the delayed setting of the optimizer based on the search space as this seems simpler to me and follows the design of the acquisition function - which makes sense, given that the optimizer is something that is also closely related to it.

[StefanPSchmid]

Thread to discuss potential removal of BoTorch Recommender:

• Considering allowing None for the Optimizer of the Recommender, I was orienting myself at the acquisition_function , which also can have None at this field and is only set later to a sensible default. I was thinking of doing the same here, i.e. in recommend there would be a new function setup_optimizer that sets a default (depending on the search space type) when the given optimizer is None. The problem from my POV currently is that I am still trying to make it work that when the user gives some arguments to BotorchRecommender for the Optimizer (e.g. sequential_continuous), that they are arguments of the Recommender, and not available at the setup stage (if I understood the code directly).
• I think for that matter too, I would be for removing the BotorchRecommender, that would move the dispatching based on searchspace type into BayesianRecommender and that gets then optimized with a specific Optimizer. Then there would also not be this dychtomy where these arguments should be. One thing of which I am very unsure is how this would be handled with backwards compatibility, currently trying to get it to work even when old interface of BotorchRecommender is called (see point above).

So to summarize - I would remove the BotorchRecommender, put the dispatching logic into BayesianRecommender and implement a _setup_acquisition_function style function for the optimizer too. What do you think?

[AVHopp]

I think @AdrianSosic already stated that he'd support the removal of the BoTorchRecommender. However, my feeling is that this might heavily depend on how the Protocols/Interface change, so we should wait with this decision in my opinion.

[AVHopp]

Give this a Flag about the supported SearchSpaceType and add validation for this.