Add TL override path to new GP factory - #868
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an explicit transfer-learning override on TaskParameter and updates the GP surrogate’s kernel resolution to honor that override by dispatching between IndexKernel and PositiveIndexKernel, including new error types and test coverage updates.
Changes:
- Introduce
TransferLearningModeandTaskParameter.override_transfer_learning_modeto explicitly select the task kernel type. - Add override-aware kernel resolution in
GaussianProcessSurrogate(including task-kernel attachment and incompatibility handling). - Harden reduced-searchspace behavior with a dedicated
AttributeErrorsubclass and extend tests/strategies accordingly.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
baybe/surrogates/gaussian_process/core.py |
Implements override-aware kernel dispatch/assembly and adds kernel-stripping helper. |
baybe/parameters/categorical.py |
Adds TaskParameter.override_transfer_learning_mode field. |
baybe/parameters/enum.py |
Introduces TransferLearningMode. |
baybe/parameters/__init__.py |
Re-exports TransferLearningMode. |
baybe/searchspace/core.py |
Raises UnsupportedSearchSpaceAttributeError for blocked reduced-searchspace attribute access. |
baybe/exceptions.py |
Adds IncompatibleOverrideError and UnsupportedSearchSpaceAttributeError. |
tests/test_kernel_factories.py |
Adds tests for override dispatch scenarios. |
tests/test_reduced_searchspace.py |
Adds test asserting the dedicated reduced-searchspace exception type. |
tests/hypothesis_strategies/parameters.py |
Extends TaskParameter strategy to optionally include overrides. |
tests/hypothesis_strategies/kernels.py |
Updates index-kernel strategy to include both index kernel variants. |
CHANGELOG.md |
Documents the new override feature and enum. |
Comments suppressed due to low confidence (1)
baybe/surrogates/gaussian_process/core.py:447
- In the override path, when
kernel_factoryis a callable (i.e., not aPlainGPComponentFactory), the returned BayBE kernel is accepted as-is and later converted viato_gpytorch(searchspace=searchspace)on the full search space. If that kernel hasparameter_names=None(default for many basic kernels) or otherwise still references the task parameter, it will end up acting on the task dimension as well, causing an overlap with the explicitly attached task kernel. This contradicts the intended “task-free base kernel” behavior and can yield incorrect models.
if not isinstance(factory_kernel, Kernel):
raise IncompatibleOverrideError(incompatible_message)
base_spec = factory_kernel
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
89de707 to
a7b119a
Compare
AVHopp
left a comment
There was a problem hiding this comment.
General logic looks good, but there is missing coverage for the case of factory being provided and one bug related to this
| default=None, | ||
| validator=optional(instance_of(TransferLearningMode)), | ||
| ) | ||
| """Optional override for the transfer learning mode.""" |
There was a problem hiding this comment.
Should this docstring maybe already contain some more information about how conflicts are handled?
There was a problem hiding this comment.
In general - where and how do we best document this feature/behavior? ALso in the docstring of the GPSurrogate or its fields?
There was a problem hiding this comment.
I added the main description to the TaskParameter itsefl and a brief comment to the surrogate's field: c7862ad. Are you fine with this version?
| return value | ||
|
|
||
|
|
||
| def _strip_task_from_kernel( |
There was a problem hiding this comment.
It feels weird to me that this function is a private helper within Gaussian_Process/core.py. Shouldn't it eiter live directly with kernels or at some utility function for kernels?
There was a problem hiding this comment.
I moved the helper to kernels: ae3c5c5. I introduced a new method for kernels _without_parameter that will return the same kernel operating on the reduced set of parameters for all kernels for which we can easily define the reduces version. Other kernels, like additive ones will raise a TypeError. Does this work for you?
| return _ModelContext(searchspace, objective, measurements) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
Aren't we missing a test case for having a factory here? I only see None and explicit kernels
There was a problem hiding this comment.
I added a test for factories returning gpytorch kernels and task-free BayBE kernels here: 42fc1c7. For factories returning task-aware BayBE factories see your other comment.
There was a problem hiding this comment.
Added a test for factories returning task-aware BayBE factories here: bfa8983.
| encoding: CategoricalEncoding = field(default=CategoricalEncoding.INT, init=False) | ||
| # See base class. | ||
|
|
||
| override_transfer_learning_mode: TransferLearningMode | None = field( |
There was a problem hiding this comment.
This field is ignored for all other models that are not GPSurrogates, should this be flagged/mentioned somewhere/raise an error/warning?
There was a problem hiding this comment.
Good point. The thing is that currently GPSurrogate is the only surrogate that has supports_transfer_learning=True and can work with a TaskParameter. Any other surrogate will already raise an ValueError if combined with a TaskParameter and the override field will never be ignored. If we ever add another surrogate for TL that can not use the override we should definitely coem back to this.
| ) | ||
|
|
||
| task_kernel = task_kernel_spec.to_gpytorch(searchspace=searchspace) | ||
| return task_kernel if base_kernel is None else base_kernel * task_kernel |
There was a problem hiding this comment.
There is an issue here in the case of a factory which produces a kernel that has parameter_names=None: In this case, we land in the "else" branch above, and factory_kernel is then a kernel built on the reduced search space, but with parameter_names=None, so it is unclear that this should be "task-free". Thus, the moment we set base_kernel, this kernel spans all columns of the search space, including the task-related one. But we still multiply the task kernel, hence modelling the task twice.
There was a problem hiding this comment.
Code demonstrating this:
def _unnamed_kernel_factory(searchspace, objective, measurements):
"""A callable factory returning a BayBE kernel with `parameter_names=None`."""
return MaternKernel() # parameter_names defaults to None -> "acts on all dims"
def test_pathD_double_covers_task_column(monkeypatch):
"""A callable factory returning an unnamed kernel double-covers the task column."""
monkeypatch.setenv("BAYBE_DISABLE_CUSTOM_KERNEL_WARNING", "True")
task = TaskParameter(
"Task",
["A", "B", "C"],
active_values=["A"],
override_transfer_learning_mode=TransferLearningMode.INDEX_KERNEL,
)
num = NumericalDiscreteParameter("x", [1, 2, 3, 4, 5])
searchspace = SearchSpace.from_product([num, task])
context = _ModelContext(
searchspace, NumericalTarget("y").to_objective(), pd.DataFrame()
)
task_idx = context.task_idx
surrogate = GaussianProcessSurrogate(kernel_or_factory=_unnamed_kernel_factory)
kernel = surrogate._resolve_kernel(context)
base_kernel, task_kernel = kernel.kernels
print(f"\ntask column index = {task_idx}")
print(f"base_kernel.active_dims = {base_kernel.active_dims}")
print(f"task_kernel.active_dims = {task_kernel.active_dims}")
# The task kernel correctly acts only on the task column.
assert set(task_kernel.active_dims.tolist()) == {task_idx}
# In gpytorch, `active_dims is None` means "acts on every input dimension" -
# i.e. the base kernel also covers the task column, modeling it twice. A
# properly task-free base kernel must instead be restricted to a subset.
assert base_kernel.active_dims is not NoneWhen this is added as a test, it fails
There was a problem hiding this comment.
Added a fix here: bfa8983. A factory with parameter_names=None is now reduced to the non-task paremeters. Included a test.
There was a problem hiding this comment.
Good catch btw :) Thanks!
Scienfitz
left a comment
There was a problem hiding this comment.
not ready for review, please rebase
…a TaskParameter - Change the default transfer learning kernel from `IndexKernel` to `PositiveIndexKernel`, enforcing positive task correlations; `PositiveIndexKernel` disables BoTorch's target-task normalization (`unit_scale_for_target=False`) - Add `TransferLearningMode` enum (`INDEX_KERNEL`, `POSITIVE_INDEX_KERNEL`) in `baybe/parameters/enum.py` - Add optional `TaskParameter.override_transfer_learning_mode` field (default `None`) to override the task kernel used for transfer learning - Add `GaussianProcessSurrogate._resolve_kernel`: when an override is set, strip the task parameter, run the kernel factory on the reduced search space, and attach the requested task kernel - Add `SearchSpace._without_task_parameter()` helper to build a task-free search space - Raise the new `IncompatibleKernelError` when an override clashes with a task-aware kernel factory - Extend the `task_parameters` and kernel hypothesis strategies to cover the new field and `PositiveIndexKernel`; add tests for factory dispatch and `_resolve_kernel` - Use the default transfer-learning kernel in the transfer-learning benchmarks - Filter the "Negative variance values detected" `NumericalWarning` in `pytest.ini`, add a `gpytorch` intersphinx mapping for the docs, and update the CHANGELOG Co-authored-by: Martin Fitzner <martin.fitzner@merckgroup.com>
- build the base kernel from what the user gave us and attach the requested task kernel manually - Strip the task out of user-provided (scaled) kernels - Raise when the override can't be implementded (raw gpytorch kernels) - Add a dedicated error for blocked reduced-search-space access so we catch the right thing instead of an general AttributeError - Update the tests for the new behavior
- Undo unintended reformat - Remove test for defautls - Export TransferLearningMode at top level namespace - Improve docstring - Rename IncompatibleKErnelError to IncompatibleOverrideError - Make IndexKernel case explicit
- The override used to build the base kernel on a reduced search space, where the default factory's numerical kernel cannot resolve its active dimensions, so it raised on non-substance spaces. - Route the default factory through the ICM machinery on the full search space instead, pairing the task-excluded base kernel with the requested index kernel. - Leave the reduced-search-space path for other factories and the strip path for fixed kernels untouched, so all other behavior stays the same. - Move the default-factory override cases from the raising test to the success test.
a7b119a to
c3f7efd
Compare
There was a problem hiding this comment.
could you amend and squash some commits such that DISPATCHING_PLAN does not appear anymore at all in this PR?
There was a problem hiding this comment.
Will squash all commits in the end and update commit message.
There was a problem hiding this comment.
please make your agent respect AGENTS.md more, in particular the instruction to not have blown-up commit bodies with redundant information
Example: Rename X into Y, this does not have to be written in the commit body, its apparent in the code and provides no additional info
There was a problem hiding this comment.
Will squash all commits in the end and update commit message.
| Args: | ||
| kernel: The kernel whose task dependence should be removed. | ||
| task_name: The name of the task parameter to strip. | ||
| non_task_names: The names of all non-task parameters, used when the kernel |
There was a problem hiding this comment.
missing guards for non_task_names, eg could be empty
is there a slightly different design possible that wouldnt require to provide both sets of names?
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
202e2fa to
bfa8983
Compare
There was a problem hiding this comment.
weve tests covering the construction, but could it also be worth to add 1 simple test that uses an override in test_iterations?
| self.kernel_factory is None | ||
| or type(self.kernel_factory) is BayBEKernelFactory | ||
| ): | ||
| icm = ICMKernelFactory(task_kernel_or_factory=task_kernel_spec) |
There was a problem hiding this comment.
im not entirely sure myself, but what about the parameter_selector the baybekernelfactory has? do we need to pass that on or ignore it like done here?
| # cannot resolve its active dimensions. | ||
| if ( | ||
| self.kernel_factory is None | ||
| or type(self.kernel_factory) is BayBEKernelFactory |
There was a problem hiding this comment.
usually would prefer isinstance or is there a reason you chose this? (derived classes would fail this check)
|
|
||
|
|
||
| class TransferLearningMode(Enum): | ||
| """Transfer learning modes for `TaskParameter`.""" |
There was a problem hiding this comment.
does this turn intoa. proper reference? or do we need somtehing else like eg " :class:.TaskParameter"
| else: | ||
| # Stripping left no non-task parameters -> only the task kernel remains | ||
| base_kernel, task_kernel = None, kernel | ||
| assert type(task_kernel).__name__ == expected_task_kernel_cls |
There was a problem hiding this comment.
also here isinstance is preferred over name tests
Override for dispatching between
PositiveIndexKernelandIndexKernelforTaskParameter1) Optional
override_transfer_learning_modeonTaskParameterA
TaskParametercan now carry an optionaloverride_transfer_learning_mode. When left at its default (None), the kernel factory controls how the task dimension is treated. When set, it instructs the GP surrogateto attach the requested task kernel:
2) Dispatch in
GaussianProcessSurrogate._resolve_kernela) No override — the surrogate uses the kernel factory on the full search space. Unchanged behavior.
b) Override set — the surrogate constructs the requested
IndexKernel / PositiveIndexKernelon the task column and multiplies it with a task-free base kernel. How that base kernel is obtained depends on what was passed askernel_or_factory:Case 1 (
NoneorBayBEKernelFactory):BayBEKernelFactorydispatches internally between_ChenNumericalKernelFactory(returns a BayBE Kernel) and_CustomScaledNumericalKernelFactory(returns a rawgpytorchkernel and callsget_comp_rep_parameter_indices). The reduced searchspace blocksget_comp_rep_parameter_indicesand we can not reduce thegpytorchcomponent to the non-task parameters. The surrogate therefore routes throughICMKernelFactoryon the full search space with the prescribed task kernel directly.Case 2 (fixed kernel object (
PlainGPComponentFactory)):PlainGPComponentFactoryignores the searchspace entirely, so calling it on a reduced space returns the same object unchanged. The task parameter is instead stripped directly from the kernel via_strip_task_from_kernel, which removes the task name fromparameter_namesonBasicKernelsand single-levelScaleKernels, and raises for other composite kernels.Case 3 (callable factory): The factory is called on a task-free reduced search space (
SearchSpace._drop_parameters). Since the task parameter is absent, the factory produces atask-free kernel. The result must be a BayBE Kernel. If the output is a raw
gpytorchkernel it raisesIncompatibleOverrideError.In all override cases the base kernel's
to_gpytorchis called on the full search space, so itsactive_dimsalign with the actual training tensor, and the task kernel is attached on the task column.Raises
IncompatibleOverrideErrorfor: rawgpytorchkernels, composite kernels other thanScaleJKernel, task-aware factories, and factories that do not yield a BayBE kernel on the reduced space.3) Tests
The dispatch logic is exercised directly in
tests/test_kernel_factories.py, following the following scenarioskernel_or_factory(with override)MaternKernel(parameter_names=("x", "Task"))MaternKernel(parameter_names=("x",))MaternKernel()ScaleKernel(MaternKernel())gpytorch.kernels.MaternKernel(active_dims=[0, 1])IncompatibleOverrideErrorgpytorch.kernels.MaternKernel()IncompatibleOverrideErrorICMKernelFactory(...)(task-aware factory)IncompatibleOverrideErrorBayBEKernelFactory()(no substance parameter)ICMKernelFactory)IndexKernel(parameter_names=("Task",))MaternKernel(...) * IndexKernel(...)(product kernel)IncompatibleOverrideError