Skip to content
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

Botorch with cardinality constraint via sampling #301

Draft
wants to merge 70 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
70 commits
Select commit Hold shift + click to select a range
d811240
Enable cardinality constraint in botorch recommender via sampling ina…
Waschenbacher Jul 2, 2024
da813f5
Make inactive parameters fixed features
Waschenbacher Jul 2, 2024
adf5cc2
Fix bug in test file
Waschenbacher Jul 4, 2024
e69ceff
Validate bounds of cardinality constraint parameters
Waschenbacher Jul 2, 2024
2270350
Add second option: iterate through combinatorial list
Waschenbacher Jul 3, 2024
6483e4b
Fix type error
Waschenbacher Jul 3, 2024
ae919d4
Revise botorch+cardinality constraint for enhanced clarity
Waschenbacher Jul 4, 2024
9ab8fda
Fix property names and its docstrings
Waschenbacher Jul 11, 2024
c3831c7
Use guard clause
Waschenbacher Jul 11, 2024
2f49f5a
Simplify syntax with 'prod'
Waschenbacher Jul 11, 2024
d46fd60
Refactor botorch+cardinality constraint
Waschenbacher Jul 12, 2024
293e2ef
Make 'n_threshold_inactive_parameters_generator' an attribute of boto…
Waschenbacher Jul 12, 2024
f8d0713
Refactor combinatorial properties of cardinality constraint
AdrianSosic Aug 15, 2024
76b5d72
Refactor combinatorial properties of continuous subspace
AdrianSosic Aug 15, 2024
5c92079
Refactor constraint validation
AdrianSosic Aug 15, 2024
f07a452
Move factory code up
AdrianSosic Aug 15, 2024
c5b014d
Simplify constructor code
AdrianSosic Aug 15, 2024
306c9d2
Update CHANGELOG.md
AdrianSosic Aug 15, 2024
7687e39
Ensure active parameters by altering parameters bounds
Waschenbacher Aug 23, 2024
66c3278
Fix continuous constraint test
Waschenbacher Aug 23, 2024
a1d11e7
Refactor botorch interface using fixed parameter class
Waschenbacher Aug 23, 2024
22fd942
Add try-except block to handle infeasible problem at certain inactive…
Waschenbacher Aug 26, 2024
120d717
Update CHANGELOG.md
Waschenbacher Aug 26, 2024
e6248b5
Fix type hint
Waschenbacher Aug 26, 2024
e0508b9
Fix test by repacing match text
Waschenbacher Aug 26, 2024
04d89d1
Refine docstrings
AdrianSosic Oct 15, 2024
102ef07
Fix method return type
AdrianSosic Oct 15, 2024
e357c95
Merge branch 'main' into feature/cardinality_constraint_to_botorch_vi…
AdrianSosic Oct 25, 2024
3d72f04
Fix capitalization in exception group
AdrianSosic Oct 25, 2024
ed8054b
Add explicit error handling to validator
AdrianSosic Oct 25, 2024
756bb09
Clean up cardinality constraint helper property
AdrianSosic Oct 25, 2024
b0c422e
Refactor parameter activation logic
AdrianSosic Oct 25, 2024
ddabcf5
Refactor method for enforcing cardinality constraints
AdrianSosic Oct 25, 2024
b8d24d7
Fix exception types and messages
AdrianSosic Oct 28, 2024
492bb3b
Apply minor formatting and documentation fixes
AdrianSosic Oct 28, 2024
584d8d9
Remove unnecessary `len` call
AdrianSosic Oct 28, 2024
5744e31
Remove unnecessary function layer
AdrianSosic Oct 28, 2024
e4afdcc
Extract loop into general function optimizing subspaces
AdrianSosic Oct 28, 2024
788a5ba
Simplify multi-space optimization logic
AdrianSosic Oct 29, 2024
046a8e2
Remove restriction on subspaces without cardinality constraints
AdrianSosic Oct 29, 2024
7aac4d3
Move __str__ method to top
AdrianSosic Oct 29, 2024
3c829d0
Rename threshold attribute
AdrianSosic Oct 29, 2024
bc697c0
Add item to README.md
AdrianSosic Oct 30, 2024
b9038b8
Implement summary method
AdrianSosic Oct 30, 2024
c2c8b99
Update CHANGELOG.md
AdrianSosic Nov 1, 2024
a721f21
Fix tests
AdrianSosic Nov 1, 2024
e84dda9
Explain mechanism of recommending with cardinality constraints
AdrianSosic Nov 1, 2024
fa13267
Add near-zero threshold to continuous numerical parameter
Waschenbacher Dec 13, 2024
7aa7c3f
Refine activate parameter helper function
Waschenbacher Dec 13, 2024
cfdf1e3
Show warnings when any minimum cardinality constraints are violated.
Waschenbacher Dec 14, 2024
e3c6620
Update test related to cardinality constraints
Waschenbacher Dec 15, 2024
b85924f
Add to-dos
Waschenbacher Dec 15, 2024
22b19f9
Add test on catching warning related to violation of minimum cardinal…
Waschenbacher Dec 16, 2024
b0dc037
Update CHANGELOG.md
Waschenbacher Dec 16, 2024
3fa8b02
Merge branch 'main' into feature/cardinality_constraint_to_botorch_vi…
Waschenbacher Dec 16, 2024
35825b8
Clean up merge conflict code
Waschenbacher Dec 16, 2024
3e275e4
Refine logic in counting the near-zero elements
Waschenbacher Dec 16, 2024
62f0ed6
Add TODO related to customized infeasibility error in botorch
Waschenbacher Jan 8, 2025
bbc32b8
Add threshold to continuous cardinality constraint
Waschenbacher Jan 8, 2025
6048e70
Improve docstring of get_threshold method
Waschenbacher Jan 8, 2025
1e77053
Adapt activate_parameter towards threshold per cardinality constraints
Waschenbacher Jan 8, 2025
29a020e
Refine check cardinaltiy constraint fulfillment logic
Waschenbacher Jan 8, 2025
3761201
Remove threshold related attribute and method in numerical continuous…
Waschenbacher Jan 9, 2025
5e35585
Make zero-checking and threshold definition compatible
Waschenbacher Jan 9, 2025
38c1ee0
Handle edge-case: activate parameter leads to parameter with fixed value
Waschenbacher Jan 9, 2025
a71f75b
Remove not needed argument "threshold"
Waschenbacher Jan 9, 2025
0e1727f
Add activate parameter step in random sampler
Waschenbacher Jan 9, 2025
5bb6e8a
Fix or operator
Waschenbacher Jan 9, 2025
5f80310
Update CHANGELOG.md
Waschenbacher Jan 9, 2025
ee54c89
Fix type hint in continuous numerical parameter classes
Waschenbacher Jan 9, 2025
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
5 changes: 5 additions & 0 deletions CHANGELOG.md
AVHopp marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
## [Unreleased]
### Added
- `py.typed` file to enable the use of type checkers on the user side
- `ContinuousCardinalityConstraint` is now compatible with `BotorchRecommender`
- Utilities `inactive_parameter_combinations` and`n_inactive_parameter_combinations`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think these as well as the utilities noted below are not user-facing, are they? In that case, I do not think that it is necessary to include them in the CHANGELOG

in both `ContinuousCardinalityConstraint`and `SubspaceContinuous`
- Attribute `n_threshold_inactive_parameters_generator` added to `BotorchRecommender`
AdrianSosic marked this conversation as resolved.
Show resolved Hide resolved
- Class `_FixedNumericalContinuousParameter`
AdrianSosic marked this conversation as resolved.
Show resolved Hide resolved

### Fixed
- `CategoricalParameter` and `TaskParameter` no longer incorrectly coerce a single
Expand Down
23 changes: 23 additions & 0 deletions baybe/constraints/continuous.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
"""Continuous constraints."""

import math
from collections.abc import Iterator
from itertools import combinations
from math import comb

import numpy as np
from attrs import define
Expand Down Expand Up @@ -46,6 +49,26 @@ class ContinuousCardinalityConstraint(
):
"""Class for continuous cardinality constraints."""

@property
def n_inactive_parameter_combinations(self) -> int:
"""The number of possible inactive parameter combinations."""
Scienfitz marked this conversation as resolved.
Show resolved Hide resolved
return sum(
AVHopp marked this conversation as resolved.
Show resolved Hide resolved
comb(len(self.parameters), n_inactive_parameters)
for n_inactive_parameters in self._inactive_set_sizes()
)

def _inactive_set_sizes(self) -> range:
AdrianSosic marked this conversation as resolved.
Show resolved Hide resolved
"""Get all possible sizes of inactive parameter sets."""
return range(
len(self.parameters) - self.max_cardinality,
len(self.parameters) - self.min_cardinality + 1,
)

def inactive_parameter_combinations(self) -> Iterator[frozenset[str]]:
"""Get an iterator over all possible combinations of inactive parameters."""
for n_inactive_parameters in self._inactive_set_sizes():
yield from combinations(self.parameters, n_inactive_parameters)

def sample_inactive_parameters(self, batch_size: int = 1) -> list[set[str]]:
"""Sample sets of inactive parameters according to the cardinality constraints.

Expand Down
52 changes: 52 additions & 0 deletions baybe/constraints/validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,14 @@
from baybe.constraints.discrete import (
DiscreteDependenciesConstraint,
)
from baybe.parameters import NumericalContinuousParameter
from baybe.parameters.base import Parameter

try: # For python < 3.11, use the exceptiongroup backport
ExceptionGroup
except NameError:
from exceptiongroup import ExceptionGroup


def validate_constraints( # noqa: DOC101, DOC103
constraints: Collection[Constraint], parameters: Collection[Parameter]
Expand All @@ -26,6 +32,8 @@ def validate_constraints( # noqa: DOC101, DOC103
ValueError: If any discrete constraint includes a continuous parameter.
ValueError: If any discrete constraint that is valid only for numerical
discrete parameters includes non-numerical discrete parameters.
ValueError: If any parameter affected by a cardinality constraint does
AVHopp marked this conversation as resolved.
Show resolved Hide resolved
not include zero.
"""
if sum(isinstance(itm, DiscreteDependenciesConstraint) for itm in constraints) > 1:
raise ValueError(
Expand All @@ -41,6 +49,9 @@ def validate_constraints( # noqa: DOC101, DOC103
param_names_discrete = [p.name for p in parameters if p.is_discrete]
param_names_continuous = [p.name for p in parameters if p.is_continuous]
param_names_non_numerical = [p.name for p in parameters if not p.is_numerical]
params_continuous: list[NumericalContinuousParameter] = [
p for p in parameters if isinstance(p, NumericalContinuousParameter)
Copy link
Collaborator

Choose a reason for hiding this comment

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

would reutilzie the list param_names_continuous from above

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if that's too helpful, since it contains only the names. Or what exactly do you suggest?

Copy link
Collaborator

Choose a reason for hiding this comment

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

.get_parameters_by_name

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@AdrianSosic I didn't find the method mentioned above. In general .get_parameters_by_name can be really handy at several places. Let me know pls, if it is already implemented somewhere or if it should be added in this PR (I can add it).

Copy link
Collaborator

Choose a reason for hiding this comment

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

]

for constraint in constraints:
if not all(p in param_names_all for p in constraint.parameters):
Expand Down Expand Up @@ -78,6 +89,11 @@ def validate_constraints( # noqa: DOC101, DOC103
f"Parameter list of the affected constraint: {constraint.parameters}."
)

if isinstance(constraint, ContinuousCardinalityConstraint):
validate_cardinality_constraint_parameter_bounds(
constraint, params_continuous
)


def validate_cardinality_constraints_are_nonoverlapping(
constraints: Collection[ContinuousCardinalityConstraint],
Expand All @@ -98,3 +114,39 @@ def validate_cardinality_constraints_are_nonoverlapping(
f"cannot share the same parameters. Found the following overlapping "
f"parameter sets: {s1}, {s2}."
)


def validate_cardinality_constraint_parameter_bounds(
constraint: ContinuousCardinalityConstraint,
parameters: Collection[NumericalContinuousParameter],
) -> None:
"""Validate that all parameters of a continuous cardinality constraint include zero.

Args:
constraint: A continuous cardinality constraint.
parameters: A collection of parameters, including those affected by the
constraint.

Raises:
ValueError: If one of the affected parameters does not include zero.
ExceptionGroup: If several of the affected parameters do not include zero.
AVHopp marked this conversation as resolved.
Show resolved Hide resolved
"""
exceptions = []
for name in constraint.parameters:
# We implicitly assume that the corresponding parameter exists
parameter = next(p for p in parameters if p.name == name)

if not parameter.is_in_range(0.0):
exceptions.append(
ValueError(
f"The bounds of all parameters affected by a constraint of type "
f"'{ContinuousCardinalityConstraint.__name__}' must include zero, "
f"but the bounds of parameter '{name}' are: "
f"{parameter.bounds.to_tuple()}"
)
)

if exceptions:
if len(exceptions) == 1:
raise exceptions[0]
raise ExceptionGroup("invalid parameter bounds", exceptions)
AdrianSosic marked this conversation as resolved.
Show resolved Hide resolved
23 changes: 23 additions & 0 deletions baybe/parameters/numerical.py
Original file line number Diff line number Diff line change
Expand Up @@ -142,3 +142,26 @@ def summary(self) -> dict: # noqa: D102
Upper_Bound=self.bounds.upper,
)
return param_dict


@define(frozen=True, slots=False)
class _FixedNumericalContinuousParameter(ContinuousParameter):
Copy link
Collaborator

Choose a reason for hiding this comment

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

@AVHopp @Scienfitz: The class proved to be useful for this PR, even though we could have implemented the mechanism in a different way where instead of fixing parameter values we could have created reduced spaces where the parameter values are dropped. However, since botorch's optimizers accept a list of fixed values, this was the simpler implementation.

I've kept it private for now because I first want to see if/how things might need to be adjusted. For example, just yesterday, I had a conversation with @julianStreibel about another feature that would be very useful in the future, namely conditioning recommendations onto certain subspaces without having to redefine the searchspace object. I guess we'll use it for that as well, but until then I'll keep it hidden.

Any comments, thoughts? If not, just give thumbs up and I will resolve

Copy link
Collaborator

Choose a reason for hiding this comment

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

Iirc, we agreed that we can keep this class in the BaybAThon, right? So can this comment be resolved?

"""Parameter class for fixed numerical parameters."""

is_numeric: ClassVar[bool] = True
# See base class.

value: float = field(converter=float)
"""The fixed value of the parameter."""

@property
def bounds(self) -> Interval:
"""The value of the parameter as a degenerate interval."""
return Interval(self.value, self.value)

def is_in_range(self, item: float) -> bool:
# See base class.
return item == self.value
AVHopp marked this conversation as resolved.
Show resolved Hide resolved

def summary(self) -> dict:
raise NotImplementedError()
AdrianSosic marked this conversation as resolved.
Show resolved Hide resolved
178 changes: 170 additions & 8 deletions baybe/recommenders/pure/bayesian/botorch.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,19 @@
"""Botorch recommender."""

from __future__ import annotations

import math
from typing import Any, ClassVar
from collections.abc import Collection
from typing import TYPE_CHECKING, Any, ClassVar

import pandas as pd
from attr.converters import optional
from attrs import define, field
from attrs.validators import ge, instance_of

from baybe.constraints import ContinuousCardinalityConstraint
from baybe.exceptions import NoMCAcquisitionFunctionError
from baybe.parameters.numerical import _FixedNumericalContinuousParameter
from baybe.recommenders.pure.bayesian.base import BayesianRecommender
from baybe.searchspace import (
SearchSpace,
Expand All @@ -21,6 +27,9 @@
sample_numerical_df,
)

if TYPE_CHECKING:
from torch import Tensor


@define(kw_only=True)
class BotorchRecommender(BayesianRecommender):
Expand Down Expand Up @@ -58,6 +67,16 @@ class BotorchRecommender(BayesianRecommender):
"""Percentage of discrete search space that is sampled when performing hybrid search
space optimization. Ignored when ``hybrid_sampler="None"``."""

n_threshold_inactive_parameters_generator: int = field(
default=10, validator=[instance_of(int), ge(1)]
)
"""Threshold used for checking which inactive parameters generator is used when
cardinality constraints are present. When the size of the combinatorial list of
all possible inactive parameters is larger than the threshold, a fixed number of
randomly generated inactive parameter configurations are used and the best
optimum among them is recommended; Otherwise, we find the best one by iterating the
combinatorial list of all possible inactive parameters """

@sampling_percentage.validator
def _validate_percentage( # noqa: DOC101, DOC103
self, _: Any, value: float
Expand Down Expand Up @@ -151,31 +170,174 @@ def _recommend_continuous(
f"acquisition functions for batch sizes > 1."
)

if len(subspace_continuous.constraints_cardinality):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it necessary to have two separate functions here? I think just having a single function that just detects whether or not such constraints exist (be aware that I have no idea if there were previous discussions regarding this design)

Copy link
Collaborator

Choose a reason for hiding this comment

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

After I've had a look at these two functions, I'd propose to combine them into one: Instead of raising ValueErrors you simply execute the corresponding parts depending on whether or not the such constraints exist. I think this will not blow up the function too much, and I'd actually prefer having one slightly larger function over these two very similar functions (but happy to also hear @AdrianSosic opinion on this).

Copy link
Collaborator

Choose a reason for hiding this comment

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

are you sure its possible? one is calling the other a flexible amount of times, so the split kind of made sense to me logically

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've refactored the logic entirely. Will ping you for another review soon. Once you had a look, you can tell me here if your concern is still relevant or not

points, _ = self._recommend_continuous_with_cardinality_constraints(
subspace_continuous,
batch_size,
)
else:
points, _ = self._recommend_continuous_without_cardinality_constraints(
subspace_continuous,
batch_size,
)

# Return optimized points as dataframe
rec = pd.DataFrame(points, columns=subspace_continuous.param_names)
return rec

def _recommend_continuous_with_cardinality_constraints(
self,
subspace_continuous: SubspaceContinuous,
batch_size: int,
) -> tuple[Tensor, Tensor]:
"""Recommend from a continuous search space with cardinality constraints.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer a more detailed explanation how this works.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Added something. Now you tell me if the explanation makes sense to you or how it would need to be improved :D

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like it, only some information about how max_n_subspaces comes into play here is still missing for me :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is still the case.

Args:
subspace_continuous: The continuous subspace from which to generate
recommendations.
batch_size: The size of the recommendation batch.
Returns:
The recommendations.
The acquisition values.
Raises:
RuntimeError: If the continuous search space has no cardinality constraint.
"""
import torch

if not subspace_continuous.constraints_cardinality:
raise RuntimeError(
f"This method expects a subspace object with constraints of type "
f"{ContinuousCardinalityConstraint.__name__}. For a subspace object "
f"without constraints of type"
f" {ContinuousCardinalityConstraint.__name__}, use method"
f"{self._recommend_continuous_without_cardinality_constraints.__name__}." # noqa
)

acqf_values_all: list[Tensor] = []
points_all: list[Tensor] = []

def append_recommendation_for_inactive_parameters_setting(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you declare this as a nested function? Is there any reason to not just have this as a regular, private function that lives outside of this function?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, you seem to modify points_all within this function. If you decide to keep this as a nested function, I'd prefer if this function would simply return the calculated points_i and acqf_values_isince this avoids to change something from another scope and also makes clear where the function is "finished" (something that I always find hard to spot when using nested function)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Have completely refactored @Waschenbacher's original design. Now, we have a useful utility for optimizing collections of subspaces, that can be used on its own. Closing this thread, but feel free to ping me in the second round of review if there are further questions.

inactive_parameters: Collection[str],
):
AdrianSosic marked this conversation as resolved.
Show resolved Hide resolved
"""Append the recommendation for each inactive parameter configuration.
Args:
inactive_parameters: A list of inactive parameters.
AdrianSosic marked this conversation as resolved.
Show resolved Hide resolved
"""
# Create a new subspace by ensuring all active parameters being
AdrianSosic marked this conversation as resolved.
Show resolved Hide resolved
# non-zeros.
subspace_continuous_without_cardinality_constraints = (
subspace_continuous._remove_cardinality_constraints(inactive_parameters)
)
try:
# Optimize the acquisition function
(
points_i,
AdrianSosic marked this conversation as resolved.
Show resolved Hide resolved
acqf_values_i,
) = self._recommend_continuous_without_cardinality_constraints(
subspace_continuous_without_cardinality_constraints,
batch_size,
)
# Append recommendation list and acquisition function values
points_all.append(points_i.unsqueeze(0))
acqf_values_all.append(acqf_values_i.unsqueeze(0))

# The optimization problem may be infeasible for certain inactive
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the only case in which such an error is raised? Or is there a way to check that an error was raised due to infeasibility in constrast to other potential issues?

# parameters. The optimize_acqf raises a ValueError when the optimization
# problem is infeasible.
except ValueError:
Copy link
Collaborator

Choose a reason for hiding this comment

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

since ValueError is extremely common also for all kinds of other errors, should this be amde more precise via message?

Copy link
Collaborator

Choose a reason for hiding this comment

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

can you elaborate what you suggest in detail?

Copy link
Collaborator

Choose a reason for hiding this comment

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

simply a custom infeasibility error

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you got the context wrong: we're not raising the ValueError here – we're catching the one that botorch raises and simply skip the corresponding subspace because it's infeasible. It's the part we discussed yesterday where ideally botorch would raise an infeasibility error itself to avoid ambiguity with other errors (i.e numerical ones). But we can't raise anything here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will create a Botorch issue/feature request on customised Error for infeasibility later and let you know.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Waschenbacher saw your feature request. Just wanted to encourage you to draft a PR for this yourself if this is really a minor change. I provided two PRs myself just recently, and the guys are really helpful and it was quite straight-forward to just create a PR and get it merged. I think it took roughly a day until everything was reviewed and merged from their end.
Also, you need to sign a CLA (or similar), and I've already got the confirmation from our legal colleagues that we are officially allowed to sign this ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@AVHopp Good point. The guys approached me and asked for a PR :) . They would need it at several other positions as well. Will create a PR and let you know when it gets merged. I will take a look what is a CLA. Thanks for the insights!

Copy link
Collaborator

Choose a reason for hiding this comment

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

The CLA is basically saying "Although I wrote some code of this package, I give you all of the rights to it" - and this needs to be somehow "signed" by us. Just wanted to already tell you "You can do this, don't worry", but if you have more questions, just ask :)

pass

# Below we start recommendation
if (
subspace_continuous.n_inactive_parameter_combinations
> self.n_threshold_inactive_parameters_generator
):
# When the combinatorial list is too large, randomly set some parameters
# inactive.
for _ in range(self.n_threshold_inactive_parameters_generator):
inactive_params_sample = tuple(
subspace_continuous._sample_inactive_parameters(1)[0]
)
append_recommendation_for_inactive_parameters_setting(
inactive_params_sample
)
else:
# When the combinatorial list is not too large, iterate the combinatorial
# list of all possible inactive parameters.
for (
inactive_params_generator
) in subspace_continuous.inactive_parameter_combinations():
append_recommendation_for_inactive_parameters_setting(
inactive_params_generator
)

# Find the best option
points = torch.cat(points_all)[torch.argmax(torch.cat(acqf_values_all)), :]
acqf_values = torch.max(torch.cat(acqf_values_all))
return points, acqf_values

def _recommend_continuous_without_cardinality_constraints(
AdrianSosic marked this conversation as resolved.
Show resolved Hide resolved
self,
subspace_continuous: SubspaceContinuous,
batch_size: int,
) -> tuple[Tensor, Tensor]:
"""Recommend from a continuous search space without cardinality constraints.
Args:
subspace_continuous: The continuous subspace from which to generate
recommendations.
batch_size: The size of the recommendation batch.
Returns:
The recommendations.
The acquisition values.
Raises:
RuntimeError: If the continuous search space has any cardinality
constraints.
"""
import torch
from botorch.optim import optimize_acqf

points, _ = optimize_acqf(
if subspace_continuous.constraints_cardinality:
raise RuntimeError(
f"This method expects only subspace object without constraints of type "
f"{ContinuousCardinalityConstraint.__name__}. For a subspace object "
f"with constraints of type {ContinuousCardinalityConstraint.__name__}, "
f"try method {self._recommend_continuous.__name__}."
)

fixed_parameters = {
idx: p.value
for (idx, p) in enumerate(subspace_continuous.parameters)
if isinstance(p, _FixedNumericalContinuousParameter)
}

points, acqf_values = optimize_acqf(
acq_function=self._botorch_acqf,
bounds=torch.from_numpy(subspace_continuous.param_bounds_comp),
q=batch_size,
num_restarts=5, # TODO make choice for num_restarts
raw_samples=10, # TODO make choice for raw_samples
fixed_features=fixed_parameters or None,
equality_constraints=[
c.to_botorch(subspace_continuous.parameters)
for c in subspace_continuous.constraints_lin_eq
]
or None, # TODO: https://github.com/pytorch/botorch/issues/2042
or None,
# TODO: https://github.com/pytorch/botorch/issues/2042
inequality_constraints=[
c.to_botorch(subspace_continuous.parameters)
for c in subspace_continuous.constraints_lin_ineq
]
or None, # TODO: https://github.com/pytorch/botorch/issues/2042
or None,
# TODO: https://github.com/pytorch/botorch/issues/2042
sequential=self.sequential_continuous,
)

# Return optimized points as dataframe
rec = pd.DataFrame(points, columns=subspace_continuous.param_names)
return rec
return points, acqf_values
AdrianSosic marked this conversation as resolved.
Show resolved Hide resolved

def _recommend_hybrid(
self,
Expand Down
Loading