Skip to content
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion baybe/recommenders/pure/bayesian/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,12 @@
from __future__ import annotations

import gc
import warnings
from abc import ABC
from typing import TYPE_CHECKING

import pandas as pd
from attrs import define, field
from attrs import define, field, fields
from attrs.converters import optional
from typing_extensions import override

Expand All @@ -19,6 +20,7 @@
)
from baybe.objectives.base import Objective
from baybe.recommenders.pure.base import PureRecommender
from baybe.recommenders.pure.bayesian.botorch.optimizers.base import OptimizerProtocol
from baybe.searchspace import SearchSpace
from baybe.settings import Settings
from baybe.surrogates import GaussianProcessSurrogate
Expand Down Expand Up @@ -55,6 +57,12 @@ class BayesianRecommender(PureRecommender, ABC):
)
"""The acquisition function. When omitted, a default is used."""

optimizer: OptimizerProtocol | None = field(

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😆)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

alias="optimizer",
default=None,
)
"""The acquisition function optimizer."""

# TODO: The objective is currently only required for validating the recommendation
# context. Once multi-target support is complete, we might want to refactor
# the validation mechanism, e.g. by
Expand All @@ -67,6 +75,18 @@ class BayesianRecommender(PureRecommender, ABC):
_botorch_acqf = field(default=None, init=False, eq=False)
"""The induced BoTorch acquisition function."""

@property
def surrogate_model(self) -> SurrogateProtocol:
"""Deprecated!"""
warnings.warn(
f"Accessing the surrogate model via 'surrogate_model' has been "
f"deprecated. Use '{self.get_surrogate.__name__}' instead to get the "
f"trained model instance (or "
f"'{fields(type(self))._surrogate_model.name}' to access the raw object).",
DeprecationWarning,
)
return self._surrogate_model

def _get_acquisition_function(self, objective: Objective) -> AcquisitionFunction:
"""Select the appropriate default acquisition function for the given context."""
if self.acquisition_function is None:
Expand Down
37 changes: 6 additions & 31 deletions baybe/recommenders/pure/bayesian/botorch/continuous.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
)
from baybe.parameters.numerical import _FixedNumericalContinuousParameter
from baybe.searchspace import SubspaceContinuous
from baybe.utils.basic import flatten
from baybe.searchspace.core import SearchSpace

if TYPE_CHECKING:
from torch import Tensor
Expand Down Expand Up @@ -147,9 +147,6 @@ def recommend_continuous_without_cardinality_constraints(
Raises:
ValueError: If the continuous search space has cardinality constraints.
"""
import torch
from botorch.optim import optimize_acqf

if subspace_continuous.n_subsets > 0:
raise ValueError(
f"'{recommend_continuous_without_cardinality_constraints.__name__}' "
Expand Down Expand Up @@ -181,32 +178,10 @@ def recommend_continuous_without_cardinality_constraints(
# because it is unclear if the corresponding presence checks for these
# arguments is correctly implemented in all invoked BoTorch subroutines.
# For details: https://github.com/pytorch/botorch/issues/2042
points, acqf_values = optimize_acqf(
acq_function=recommender._botorch_acqf,
bounds=torch.from_numpy(
subspace_continuous.comp_rep_bounds.to_numpy(copy=True)
),
q=batch_size,
num_restarts=recommender.n_restarts,
raw_samples=recommender.n_raw_samples,
fixed_features=fixed_parameters or None,
equality_constraints=flatten(
c.to_botorch(
subspace_continuous.parameters,
batch_size=batch_size if c.is_interpoint else None,
)
for c in subspace_continuous.constraints_lin_eq
)
or None,
inequality_constraints=flatten(
c.to_botorch(
subspace_continuous.parameters,
batch_size=batch_size if c.is_interpoint else None,
)
for c in subspace_continuous.constraints_lin_ineq
)
or None,
sequential=recommender.sequential_continuous,
points, acqf_values = recommender.optimizer(
batch_size=batch_size,
acquisition_function=recommender._botorch_acqf,
searchspace=SearchSpace(continuous=subspace_continuous),
fixed_parameters=fixed_parameters,
)
assert acqf_values is not None # for mypy; guaranteed by optimize_acqf defaults
return points, acqf_values
8 changes: 8 additions & 0 deletions baybe/recommenders/pure/bayesian/botorch/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
recommend_hybrid_with_subsets,
recommend_hybrid_without_subsets,
)
from baybe.recommenders.pure.bayesian.botorch.optimizers.basic import GradientOptimizer
from baybe.searchspace import (
SearchSpace,
SearchSpaceType,
Expand Down Expand Up @@ -213,6 +214,13 @@ def _recommend_continuous(
f"acquisition functions for batch sizes > 1."
)

if self.optimizer is None:
self.optimizer = GradientOptimizer(
sequential_continuous=self.sequential_continuous,
n_restarts=self.n_restarts,
n_raw_samples=self.n_raw_samples,
)

points, _ = recommend_continuous_torch(self, subspace_continuous, batch_size)

return pd.DataFrame(points, columns=subspace_continuous.parameter_names)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
"""Acquisition function optimizers."""

from baybe.recommenders.pure.bayesian.botorch.optimizers.basic import GradientOptimizer

__all__ = [
"GradientOptimizer",
]
40 changes: 40 additions & 0 deletions baybe/recommenders/pure/bayesian/botorch/optimizers/base.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
"""Base protocol for all optimizers."""

from __future__ import annotations

from typing import TYPE_CHECKING, Protocol, runtime_checkable

from baybe.searchspace import SearchSpace

if TYPE_CHECKING:
from botorch.acquisition import AcquisitionFunction as BoAcquisitionFunction
from torch import Tensor


@runtime_checkable
class OptimizerProtocol(Protocol):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"""Type protocol specifying the interface optimizers need to implement."""

# Use slots so that derived classes also remain slotted
# See also: https://www.attrs.org/en/stable/glossary.html#term-slotted-classes
__slots__ = ()

def __call__(
Comment thread
StefanPSchmid marked this conversation as resolved.
self,
batch_size: int,
acquisition_function: BoAcquisitionFunction,
Comment thread
StefanPSchmid marked this conversation as resolved.
searchspace: SearchSpace,
fixed_parameters: dict[int, float] | None = None,
) -> tuple[Tensor, Tensor]:
"""Recommend a batch of points from the given search space.

Args:
batch_size: The size of the recommendation batch.
acquisition_function: The acquisition function to be optimized.
searchspace: The search space from which to generate recommendations.
fixed_parameters: A dictionary mapping parameter indices to fixed values.

Returns:
The recommendations and corresponding acquisition values.
"""
...
111 changes: 111 additions & 0 deletions baybe/recommenders/pure/bayesian/botorch/optimizers/basic.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
"""Low-level optimizers of acquisition functions."""

from __future__ import annotations

import gc
from typing import TYPE_CHECKING
from typing_extensions import override

from attrs import define, field
from attrs.validators import gt, instance_of

from baybe.recommenders.pure.bayesian.botorch.optimizers.base import OptimizerProtocol
from baybe.searchspace import SearchSpace
from baybe.utils.basic import flatten

if TYPE_CHECKING:
from botorch.acquisition import AcquisitionFunction as BoAcquisitionFunction
from torch import Tensor


@define(kw_only=True)
class GradientOptimizer(OptimizerProtocol):
"""Acquisition function optimizer using gradient-based optimization."""

n_restarts: int = field(validator=[instance_of(int), gt(0)], default=10)
"""Number of times gradient-based optimization is restarted from different initial
points. **Does not affect purely discrete optimization**.
"""

n_raw_samples: int = field(validator=[instance_of(int), gt(0)], default=64)
"""Number of raw samples drawn for the initialization heuristic in gradient-based
optimization. **Does not affect purely discrete optimization**.
"""

sequential_continuous: bool = field(default=True)
"""Flag defining whether to apply sequential greedy or batch optimization in
**continuous** search spaces. In discrete/hybrid spaces, sequential greedy
optimization is applied automatically.
"""

@override
def __call__(
Comment thread
AdrianSosic marked this conversation as resolved.
self,
batch_size: int,
acquisition_function: BoAcquisitionFunction,
Comment thread
AVHopp marked this conversation as resolved.
searchspace: SearchSpace,
fixed_parameters: dict[int, float] | None = None,
) -> tuple[Tensor, Tensor]:
"""Recommend from a search space using gradient-based optimization.

Args:
batch_size: The size of the recommendation batch.
acquisition_function: The acquisition function to be optimized.
searchspace: The search space from which to generate recommendations.
fixed_parameters: A dictionary mapping parameter indices to fixed values.

Returns:
The recommendations and corresponding acquisition values.

Raises:
NotImplementedError: If the search space has a discrete component.
ValueError: If the search space has cardinality constraints.
"""
import torch
from botorch.optim import optimize_acqf

if not searchspace.discrete.is_empty:
raise NotImplementedError(
"Gradient-based optimization is not implemented "
"for non-empty discrete search spaces."
)

if searchspace.continuous.n_subsets > 0:
raise ValueError(
f"'{self.__class__.__name__}' "
f"expects a continuous subspace without cardinality constraints."
)

points, acqf_values = optimize_acqf(
acq_function=acquisition_function,
bounds=torch.from_numpy(
searchspace.continuous.comp_rep_bounds.to_numpy(copy=True)
),
q=batch_size,
num_restarts=self.n_restarts,
raw_samples=self.n_raw_samples,
fixed_features=fixed_parameters or None,
equality_constraints=flatten(
c.to_botorch(
searchspace.continuous.parameters,
batch_size=batch_size if c.is_interpoint else None,
)
for c in searchspace.continuous.constraints_lin_eq
)
or None,
inequality_constraints=flatten(
c.to_botorch(
searchspace.continuous.parameters,
batch_size=batch_size if c.is_interpoint else None,
)
for c in searchspace.continuous.constraints_lin_ineq
)
or None,
sequential=self.sequential_continuous,
)

return points, acqf_values


# Collect leftover original slotted classes processed by `attrs.define`
gc.collect()
Loading