-
Notifications
You must be signed in to change notification settings - Fork 79
Add _ReducedSearchSpace for parameter-only reduced search spaces #819
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
base: main
Are you sure you want to change the base?
Changes from all commits
3e2bdef
c0289f0
c2c3f72
2a4d7f5
dae8b3c
fa31cc4
99c6f31
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,10 +3,10 @@ | |
| from __future__ import annotations | ||
|
|
||
| import gc | ||
| from collections.abc import Iterable, Iterator, Sequence | ||
| from collections.abc import Collection, Iterable, Iterator, Sequence | ||
| from enum import Enum | ||
| from itertools import product | ||
| from typing import TYPE_CHECKING, cast | ||
| from typing import TYPE_CHECKING, ClassVar, cast | ||
|
|
||
| import numpy as np | ||
| import numpy.typing as npt | ||
|
|
@@ -433,6 +433,23 @@ def get_comp_rep_parameter_indices( | |
| if col in p.comp_rep_columns | ||
| ) | ||
|
|
||
| def _get_n_comp_rep_columns( | ||
| self, | ||
| name_or_selector: str | ParameterSelectorProtocol, | ||
| /, | ||
| ) -> int: | ||
| """Get the number of comp-rep columns for a parameter selection. | ||
|
|
||
| Args: | ||
| name_or_selector: Either the name of a single parameter or a selector | ||
| that filters parameters to be included. | ||
|
|
||
| Returns: | ||
| The number of columns in the computational representation associated | ||
| with the selected parameter(s). | ||
| """ | ||
| return len(self.get_comp_rep_parameter_indices(name_or_selector)) | ||
|
|
||
| @staticmethod | ||
| def estimate_product_space_size(parameters: Iterable[Parameter]) -> MemorySize: | ||
| """Estimate an upper bound for the memory size of a product space. | ||
|
|
@@ -515,6 +532,100 @@ def get_parameters_by_name(self, names: Sequence[str]) -> tuple[Parameter, ...]: | |
| names | ||
| ) + self.continuous.get_parameters_by_name(names) | ||
|
|
||
| def _drop_parameters(self, names: Collection[str], /) -> _ReducedSearchSpace: | ||
| """Return a reduced search space without the named parameters. | ||
|
|
||
| The returned object exposes only parameter information and blocks | ||
| access to constraints, subspaces, and transformation. | ||
|
|
||
| Args: | ||
| names: The names of the parameters to remove. | ||
|
|
||
| Raises: | ||
| ValueError: If any name does not match a parameter in the space. | ||
|
|
||
| Returns: | ||
| A reduced search space containing only parameter information. | ||
| """ | ||
| current_names = {p.name for p in self.parameters} | ||
| if unknown := set(names) - current_names: | ||
| raise ValueError( | ||
| f"Parameter name(s) {unknown} not found in the search space. " | ||
| f"Available: {current_names}." | ||
| ) | ||
| remaining = [p for p in self.parameters if p.name not in set(names)] | ||
|
|
||
| disc_params = [p for p in remaining if p.is_discrete] | ||
| cont_params = [p for p in remaining if p.is_continuous] | ||
|
|
||
| # Explicit comp_rep needed because transform() drops columns for empty inputs. | ||
| discrete = ( | ||
| SubspaceDiscrete( | ||
| parameters=disc_params, | ||
| exp_rep=pd.DataFrame(columns=[p.name for p in disc_params]), | ||
| comp_rep=pd.DataFrame( | ||
| columns=[c for p in disc_params for c in p.comp_rep_columns] | ||
| ), | ||
| ) | ||
| if disc_params | ||
| else SubspaceDiscrete.empty() | ||
| ) | ||
|
|
||
| continuous = ( | ||
| SubspaceContinuous( | ||
| parameters=cont_params, | ||
| ) | ||
| if cont_params | ||
| else SubspaceContinuous.empty() | ||
| ) | ||
|
|
||
| return _ReducedSearchSpace(discrete=discrete, continuous=continuous) | ||
|
|
||
|
|
||
| @define(slots=False) | ||
| class _ReducedSearchSpace(SearchSpace): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we think that inheritance is the way to go here? This implementation clearly violates LSP, and we could probably also use
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair point. But tbh I this is hopefully only a temporary solution and I think we can live with it.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have also talked with @kalama-ai offline about a related issue, but my opinion: this is clearly a hack anyway and not a first-class new feature, so do whatever it takes to bring you to the goal as quickly as possible 😬 |
||
| """A lightweight search space exposing only parameter information. | ||
|
AdrianSosic marked this conversation as resolved.
|
||
|
|
||
| Provides access to parameter-related properties needed by kernel factory | ||
| calls. Blocks access to transformation, index-based lookups, and other | ||
| functionality requiring actual candidate data. | ||
|
|
||
| This class is not intended for direct construction. Use | ||
| :meth:`SearchSpace._drop_parameters` instead. | ||
| """ | ||
|
|
||
| _ALLOWED_ATTRIBUTES: ClassVar[frozenset[str]] = frozenset( | ||
| { | ||
| "discrete", | ||
| "continuous", | ||
| "parameters", | ||
| "parameter_names", | ||
| "comp_rep_columns", | ||
| "constraints", | ||
| "type", | ||
| "_task_parameter", | ||
| "n_tasks", | ||
| "_get_n_comp_rep_columns", | ||
| "get_comp_rep_parameter_indices", | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now you have added this one here to the list, which shouldn't be there. I know, |
||
| "get_parameters_by_name", | ||
| "_ALLOWED_ATTRIBUTES", | ||
| } | ||
|
kalama-ai marked this conversation as resolved.
|
||
| ) | ||
| """Attributes accessible on this reduced search space.""" | ||
|
|
||
| @override | ||
| def __getattribute__(self, name: str): | ||
| """Guard attribute access, allowing only parameter-related attributes.""" | ||
| if name.startswith("__"): | ||
| return object.__getattribute__(self, name) | ||
| allowed = object.__getattribute__(self, "_ALLOWED_ATTRIBUTES") | ||
| if name in allowed: | ||
| return object.__getattribute__(self, name) | ||
| raise NotImplementedError( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this rather raise an |
||
| f"'{object.__getattribute__(self, '__class__').__name__}' does not " | ||
| f"support attribute '{name}'. Only parameter information is available." | ||
| ) | ||
|
|
||
|
|
||
| def to_searchspace( | ||
| x: Parameter | SubspaceDiscrete | SubspaceContinuous | SearchSpace, / | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -311,6 +311,8 @@ def _fit(self, train_x: Tensor, train_y: Tensor) -> None: | |
| ) | ||
|
|
||
| ### Kernel | ||
| # TODO: When calling a factory on a `_ReducedSearchSpace`, validate that | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to be sure, this is intended to be done in a follow-up PR I assume and not in this one?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this was only meant to highlight where the new |
||
| # it returns a BayBE Kernel (not a raw gpytorch kernel). | ||
| kernel = self.kernel_factory( | ||
| context.searchspace, context.objective, context.measurements | ||
| ) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't you also precompute
set_names = set(names)here to avoid the double calculation in both theifand the calculation ofremaining?