Repository navigation
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a per-parameter kernel_override convenience interface to compose Gaussian process kernels at the parameter level, while keeping transfer-learning overrides compatible and warning when non-GP surrogates ignore these overrides.
Changes:
- Introduces
kernel_overrideon regular parameters (explicitly disallowed onTaskParameter) with validation and equivalence handling. - Refactors GP kernel resolution to apply (parameter + transfer-learning) overrides via a dedicated
_override/subpackage and residual-kernel composition. - Adds comprehensive tests, docs, and a changelog entry; warns when non-GP surrogates ignore configured overrides.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/validation/test_parameter_validation.py | Adds constructor-time validation coverage for invalid overrides and TaskParameter rejection. |
| tests/test_surrogate.py | Verifies non-GP surrogates emit UnusedObjectWarning when overrides are present. |
| tests/test_parameter_kernel_overrides.py | New functional test suite covering override binding/composition and incompatibility cases. |
| tests/test_iterations.py | Adds an end-to-end iteration test exercising parameter kernel overrides. |
| tests/hypothesis_strategies/parameters.py | Extends parameter strategies to optionally generate valid kernel overrides. |
| docs/components/surrogates.md | Links surrogate kernel documentation to the new parameter override section. |
| docs/components/parameters.md | Documents parameter-specific kernel overrides, semantics, and limitations. |
| CHANGELOG.md | Records the new parameter-specific kernel override feature. |
| baybe/surrogates/gaussian_process/core.py | Implements the new override-aware kernel resolution flow and residual-kernel logic. |
| baybe/surrogates/gaussian_process/_override/init.py | Exposes the private override-resolution helpers for the GP surrogate. |
| baybe/surrogates/gaussian_process/_override/core.py | Adds shared helpers for reducing kernel specs and raising incompatibility errors. |
| baybe/surrogates/gaussian_process/_override/parameter.py | Extracts and binds per-parameter overrides (BayBE and raw GPyTorch kernels). |
| baybe/surrogates/gaussian_process/_override/tl.py | Extracts and builds the transfer-learning override kernel factor. |
| baybe/surrogates/base.py | Adds supports_kernel_overrides and emits warnings when unsupported surrogates ignore overrides. |
| baybe/parameters/categorical.py | Prevents TaskParameter from exposing/accepting kernel_override. |
| baybe/parameters/base.py | Adds kernel_override field, validation/scoping converter, and updates equivalence logic. |
| baybe/kernels/composite.py | Implements _with_parameter scoping for composite kernels to support owner rebinding. |
| baybe/kernels/base.py | Adds a _with_parameter API on kernels and implements it for basic kernels. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
AVHopp
left a comment
AVHopp
left a comment
There was a problem hiding this comment.
First round of reviews - already looks quite nice :)
| return attrs.evolve(self, name=other.name) == other | ||
| # The override is owner-scoped, so rebind it to the other parameter's name. | ||
| kernel_override = self.kernel_override | ||
| if isinstance(kernel_override, Kernel): |
There was a problem hiding this comment.
What happens in the case of a GPyTorch Kernel?
There was a problem hiding this comment.
Also, Claude claims that two "seperately instantiated, but structurally identical GPyTorch overrides always compare unequal", so please double-check
There was a problem hiding this comment.
seems right, but the problem seems to eb that gpytorch kernels do not have a method equivalent to is_equivalent, hence what am i supposd to use?
simple equality just compares isntances
and state_dict does not cover everything, apparently
gk.MaternKernel(nu=0.5)
gk.MaternKernel(nu=2.5)would have the same state_dict
So the only reasonable compromise would be that I manually test for class and state_dict. It would already fail for reaosnable cases like above, but anything beyond that seems complex and more of a gpytorch topic and we could only solve it by some sort of generic introseption of the other attributes
There was a problem hiding this comment.
Stumbled over the same. So right now our is_equivalent would simply return False for any GPyTorch case, right? Can we live with that?
There was a problem hiding this comment.
I could, its not nice but the above pints out why its not that quick and easy to have it fixed
There was a problem hiding this comment.
ok I've extended the equivalence functionality now to also catch gpytorch. there are a few decisions that have to be made ie what to include in such a compariosn
for this I simply followed the premise everything that is not changed by a fit should match exactly. values (but not shapes) of things changed by the fit can differ and will not break equivalence. this results in
- if you create two instances of the same gpytorch kernel they will generally be equivalent, even if they have a different initial value for some attributes (which is done automatically due to random initialization at creation)
- even if 2 gpytorch kernels have the same value for a tensor, they are not considered equivalent if one of the kernels had it
frozenand one hadnt - those were the questionalble cases, all other cases are more or less clear, there are several of these combos now tested
There was a problem hiding this comment.
Looks good to me, thanks for taking care of this :) If @AdrianSosic agrees we can resolve this imo.
|
With this PR its also easy to try the old idea of using an index kernel for categorical parameters, see here the result for our full lookup example: |
218c656 to
8d5471e
Compare
| return attrs.evolve(self, name=other.name) == other | ||
| # The override is owner-scoped, so rebind it to the other parameter's name. | ||
| kernel_override = self.kernel_override | ||
| if isinstance(kernel_override, Kernel): |
There was a problem hiding this comment.
Stumbled over the same. So right now our is_equivalent would simply return False for any GPyTorch case, right? Can we live with that?
| encoding: CategoricalEncoding = field(default=CategoricalEncoding.INT, init=False) | ||
| # See base class. | ||
|
|
||
| kernel_override: None = field(init=False, default=None) |
There was a problem hiding this comment.
This is either a completely stupid question or potentially a game changer. I'm posting it now while I've not yet read all details but only glossed over the changes.
I see that you've added quite some machinery to handle/extract the overrides, but everything is strictly separated into parameter vs TL-overrides, i.e. there are parameter.py and tl.py. Provoking thought: A TL-override essentially is parameter override (I know, other non-kernel TL mechanisms may follow, but I don't think this breaks my argument). The only difference is that TL overrides are specified in terms of an Enum and not a KernelOverride. But can we simply adjust the attribute/property structure such that we simply expose a KernelOverride attribute for TaskParameter as well, whose return value is controlled by the enum? Intention: potentially you can then get rid of the entire TL-override logic and simply reuse the same parameter-based approach?
There was a problem hiding this comment.
had similar thoughts already, imo this is a classic lets unify things in the future and not now situation
we currently have overrides as
- specific component (currently as parameter kernel override): kernel-only with the possibility to be very specific with kernel-dependent settings. These only make sense if a component can actually be "assigned" to a parameter
- overall method choices (currently as tl override): embodied by enum, not configurable, affects possibly more than one component
it is totally thinkable that the current situation can be generalized such that both parameter and tl overrides accept both rather than jsut one of the above. But it doesnt make sense to think and design this now due to a lack of possible choices for overrides.
It will also be easy to add this in the future, just expand the type hint of the overrideattributes and make the handlers recognize them
There was a problem hiding this comment.
and enabling the kernel override for task parameters opens the questions:
- what are allowed kernels, should these be valdiated?
- clash tl override and kernel override? precedence? validate to avoid not both set?
if you can print a bulletpoint list of all validations that need to be ensured I might add it. But I think we first need to be sure if its really better to
- also add
kernel_override - OR to just have one unified override element (in the future) that holds both
method- andkernel-overrides like described in my prev post
There was a problem hiding this comment.
I think you may have understood it in a way more general sense than I had in mind 🙃 I'm specifically pointing out this idea to potentially make the current PR much simpler that it is. My naive proposal (please tell me where it breaks):
- The task parameter keeps it's current
override_transfer_learning_modeconstructor parameter as the only user-configurable entry point in form of well-defined enum values - However, it exposes a
kernel_overrideproperty in the expectedKernelOverride | Noneformat. - --> Consequence: all callers can handle the two situations identically. Specifically, (and I haven't gone through the details there), the entire
tl.pybecomes obsolete
There was a problem hiding this comment.
maybe to add: think of the patter that we use e.g. for values. Some classes expect the user to directly provide it, other express values via a different user-facing argument. But in any case, the common interface dictated is that consuming objects can assume a readable .values attribute/property exists
There was a problem hiding this comment.
ok I see, a property, that is better (in your original post it sounded like you want an attribute specifically)
without having looked through it I indeed think that would make at least some parts of tl.py obsolete. Eventually with such an approach the _override content would only be component-wise (with the first component being kernel) and not kernel, tl, mf etc
Specifically the match logic currently in tl.py would be owned by the parameter, which is nice?
There was a problem hiding this comment.
so ... if you also think it may be a good idea ... do you wanna give it a shot 🙃 Then I would delay the reviewing of these files until refactored and only look at documentation and tests
There was a problem hiding this comment.
ok I'll take a look, but it will probably not change as much as you think, it will merely add the underscore-init-property combo we already know for other components
otherwise it just shifts code, tl.py will be gone but its code lives on
There was a problem hiding this comment.
|
@Scienfitz I will probably not be able t give a full review before my holiday. I thus just want to let you know that finalizing and merging this PR while I am out of office is fine for me and that you should not be blocked by me having given an earlier review and potentially not being able to give another one in the next weeks. |
There was a problem hiding this comment.
can you share a link to rendered doc?
There was a problem hiding this comment.
| """A strategy that generates parameter categories.""" | ||
|
|
||
|
|
||
| def _remove_kernel_parameter_names(kernel): |
There was a problem hiding this comment.
you already have an _iter_basic_kernels function in the production code. You can use that one retrieve the kernels without duplicating the logic
There was a problem hiding this comment.
I extended _scope_to_parameter to also accept None (which means "unscope"), this enabled completely getting rid of one semi-redundant utility
| ], | ||
| ) | ||
| def test_partition_validation(monkeypatch, override_kind, kernel_cls, active_dims): | ||
| """Reject misbound regular and TL factors, matching ICM for TL partitions.""" |
There was a problem hiding this comment.
I appreciate that you added a proper test suite, but the test functions in this file are really not easy to understand. This one here is a good example: reading the docstring alone, I have really no clue what the test does. And looking at the body, I see some involved kernel constructions and monkeypatching...
Is there a way how we can make these tests easier for people to understand in the future? At the moment, going via the debugger was my only chance but I haven't done so for all of the tests. Maybe some simple comments and assigning intermediate variables with descriptive names?
There was a problem hiding this comment.
I've tried to keep it at bay but its all very unsatisfactory, let me completely redo the entire test suite from scratch once everything else is very logged in
8cb602f to
99081d5
Compare
kalama-ai
left a comment
kalama-ai
left a comment
There was a problem hiding this comment.
Thanks for your work, @Scienfitz. The per parameter kernel definition will be a very helpful tool imo.
| f"'{type(effective_factory).__name__}' does not satisfy this (e.g., it " | ||
| f"returns a raw gpytorch kernel or already operates on the task " | ||
| f"parameter)." | ||
| def _resolve_default_base( |
There was a problem hiding this comment.
might feel unsatisfaory to need this method here and not go simply via existing factories like BayBE or ICM
but these factories at the moment are not able to treat every situation like eg no-task, no-numerical etc. Also the _ReducedSearchSpace does not currently carry the info on the whole comp columns. If the latter is fixed and the factories are more acceptive of situations we could replace most parts of this method here and just call a factory with reduced space
but given the entire comp columns topic is removed/changed in the candidates work this topic here is deferred and we are currently living with _resolve_default_base
There was a problem hiding this comment.
I suggest to to remembrer this particular cleanup as one that should be done after the dev/canidates merge, its affecting too many other PRs
AVHopp
left a comment
AVHopp
left a comment
There was a problem hiding this comment.
Nothing major, hence approve.
Co-authored-by: AdrianSosic <adrian.sosic@merckgroup.com>
Co-authored-by: AdrianSosic <adrian.sosic@merckgroup.com>
A summand scoped exclusively to the removed parameter is dropped, which treats it as the additive identity. The alternative, retaining the degenerate constant, is not expressible since its value is a learned quantity and no constant kernel exists. Refusing the reduction does not preserve correctness either, so all composites now reduce uniformly.
Passing None removes all leaf scoping, which lets the Hypothesis strategy reuse the production traversal instead of duplicating it.
The unregularized RBF override on x2 intermittently caused NaN acquisition gradients (OptimizationGradientError) with few data points, which the retry logic of run_iterations does not catch. A lengthscale prior stabilizes the fit: 6 of 60 seeds failed without it and none with it.
Co-authored-by: Alexander V. Hopp <alexander.hopp@merckgroup.com>
a38d3da to
2cc8d56
Compare
Builds on #868, now merged into
mainkernel_overrideto regular parameters, accepting BayBE or raw GPyTorch kernel instances. Overrides cover the parameter’s full computational block.TaskParameter.override_transfer_learning_mode._overridepackage, providing a foundation for future override mechanisms.Limitations
UnusedObjectWarning.kernel_overrideaccepts kernel instances only, not factories.TaskParameterdoes not exposekernel_override; useoverride_transfer_learning_modeinstead.parameter_names=Noneor exactly the owning parameter’s name.active_dims;ard_num_dimsmust beNoneor match the parameter’s computational width.Noneis preserved. Raw overrides are not serializable.IncompatibleOverrideError. Additive-kernel reduction remains unsupported.Long Term Control Flow Vision
