GP Criterion

[AdrianSosic]

DevPR, parent is #745

Makes the optimization criterion of the GaussianProcessSurrogate model configurable, in the form of a new Criterion enum. Potentially, this might be generalized to a class-based approach in the future if more configuration options are required, but for now the simpler solution serves all existing use cases.

[Copilot]

Pull request overview

This PR makes the hyperparameter optimization criterion used by GaussianProcessSurrogate configurable by introducing a new Criterion enum and wiring it into model fitting. It extends the existing “component factory” pattern so presets and user code can select which GPyTorch MLL object is used during training.

Changes:

• Add Criterion enum (+ factory protocol/Plain factory) with a to_gpytorch(...) adapter to create the appropriate GPyTorch MLL.
• Extend GaussianProcessSurrogate to accept criterion_or_factory and use it during _fit when calling fit_gpytorch_mll.
• Update GP presets to expose a default criterion choice and export Criterion via preset/component __init__.py.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
CHANGELOG.md	Documents the new configurable GP optimization criterion feature.
baybe/surrogates/gaussian_process/components/criterion.py	Introduces Criterion and helpers to create the corresponding GPyTorch MLL.
baybe/surrogates/gaussian_process/components/generic.py	Extends component typing/validation to allow Criterion as a supported GP component.
baybe/surrogates/gaussian_process/components/init.py	Re-exports criterion-related types/factories.
baybe/surrogates/gaussian_process/core.py	Adds criterion_factory, plumbs it into _fit, and extends from_preset signature.
baybe/surrogates/gaussian_process/presets/baybe.py	Adds BayBE default criterion factory (task-aware selection).
baybe/surrogates/gaussian_process/presets/chen.py	Adds a preset criterion factory for CHEN.
baybe/surrogates/gaussian_process/presets/edbo.py	Adds a preset criterion factory for EDBO.
baybe/surrogates/gaussian_process/presets/edbo_smoothed.py	Adds a preset criterion factory for smoothed EDBO.
baybe/surrogates/gaussian_process/presets/init.py	Exports Criterion and the preset criterion factories.

Comments suppressed due to low confidence (1)

baybe/surrogates/gaussian_process/presets/baybe.py:112

• PresetCriterionFactory is set to the BayBECriterionFactory class rather than an instance. Given from_preset currently does not instantiate PresetCriterionFactory, this will pass a class through and later fail when invoked with (searchspace, train_x, train_y). Set PresetCriterionFactory = BayBECriterionFactory() (or adjust from_preset to instantiate).

# Aliases for generic preset imports
PresetKernelFactory = BayBEKernelFactory
PresetMeanFactory = BayBEMeanFactory
PresetLikelihoodFactory = BayBELikelihoodFactory
PresetCriterionFactory = BayBECriterionFactory

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[AVHopp]

Please verify and fix the missing ()

[AVHopp]

Currently, only the usage of the BayBE preset will automatically exchange the criterion to use LEAVE_ONE_OUT, everything else will always use MARGINAL_LOG_LIKELIHOOD, independent on whether or not there is a TaskParameter correct? Was this also the case before, and is this what we want?

[AdrianSosic]

Before, no presets existed! And the only logical thing to do is to choose the criterion according to what the corresponding papers actually did / dictate. For EDBO, this is MLL. The only open question is what we want to do for CHEN. Back then, they used BAYBE (but didn't do any transfer learning). So two options:

1. They effectively used MLL, which we could hardcode
2. However, they also used whatever baybe uses. And I think in their paper, they make no claim about the criterion (can double check). So it would thus actually make more sense to apply our policy that should be used whenever a preset does not dictate a particular aspect of the GP, which I think should then be use baybe defaults, which may also change over time

So overall, I'm in favor of 2 and would thus add one more commit changing the default to baybe mode. I'm now doing the same for Botorch, btw, which also doesn't specify any "the criterion". Thoughts? @Scienfitz?

[AVHopp]

Going with option 2 is fine for me :)

[Scienfitz]

option 2 is the only reasonable choice

[AdrianSosic]

Hm, I just checked their paper again (and also Hvarfner one), and the thing is that they all do argue on the basis of the MLL 🤔 So in a sense, their methodology was built with that as the optimization criterion in mind, albeit that was probably just because that is sort of the "default" choice and they didn't explicitly consider alternatives.

So in that sense, option 1 makes equally sense to me know. Sorry for reiterating, but: thoughts? @AVHopp @Scienfitz

[Scienfitz]

Did they argue specifically for a case where an index kernel is used for TL? Or was TL not a consideration at all in the work?

If Yes: Implement their choice.

If No: We are supposed to fill that case, there is no danger of "deviation" from their template because they had no template for that situation. The original argument for making the MLL/LOOPL distinction is not changed then and needs not to be changed randomly. Option 2.

[AdrianSosic]

I don't think TL does even to be considered here. The real question is: did they use MLL because they deliberately decided or did they just happen to use it because it was the default we dictated for their setting. I'd challenge the changed randomly in that now is the first time where we ever actually think about the criterion in the context of the presets. Maybe another option: we could simply ask them once again – after all, it's their choice?

[Scienfitz]

what, the TL case is the only thing that needs to be debated here xD

the non -TL case is 100% consistent because our template also uses MLL in that case, no difference, no debate?

[AdrianSosic]

Think I disagree. Let's assume – for the argument – that our baybe default also simply used MLL in all cases. Even then, it's a choice if you want to semantically hard-code CHEN against BayBE or against the MLL class directly. In the first case, values are identical and CHEN/BayBE are semantically identical. In the latter case, only the values are identical. So if BayBE settings move, CHEN stays

[Scienfitz]

let me phrase it like this: We have a special rule that came from acknowledging that we build the kernel for the TL case as a special index kernel that is multiplied, the investiation made the choice to use the LOO

ie we have a special rule for the TL case, because we investigated it. It is a special rule only for the TL case.

I assume none of the papers for which youre now creating presets even talks about the case where different kernel types are combined, especially not for the TL case we investigated. So why would you then take these presets and overwrite our special rule while the presets never investigated the situation we investigated to create the special rule?

in the case the special rule doesnt apply (ie non-TL) the used MLL coincides with the preset used MLL, ie there is absolutely nothing to discuss for the non-TL case?

[AVHopp]

Re-iterating on Copilot's comment, don't you miss the () at the end of this line as this is also present for the other factories?

[AdrianSosic]

yes, noticed myself a minute ago 😬 will fix

[AVHopp]

Or is this not necessary due to Criterion being an Enum and you only ever calling .to_gpytorch anyway?

[AdrianSosic]

no is needed. Because this is not yet the enum, but the factory creating the enum value

[Scienfitz]

lines above dont use brackets for the equivalent objects, would be strange if this differed by design

[AdrianSosic]

I just noticed that the way the "preset" objects in the files were defines was not consistent across type, i.e. some referred to class, others to objects.

Here a proposal how I'd harmonize the name (--> effectively they are module constants, so uppercase is actually nice): f213893

What remains a bit wird is that some of the other variables in the file are mixed instance/type. For example, EDBOLikelihoodFactory is a class while EDBOCriterionFactory is the actual factory. Any suggestions here, do we want to live with this?

[Scienfitz]

I dont get these changes at first glance, the variable is supposed to store a factory, so why are they now evaluated with () when assigned to the constants? Did you build factories that create factories?

[AdrianSosic]

No, but their definition is class-based – I was also a bit confused, but that's simply how things are:

If you write something like class Car, then you'd not consider Car to be the actual car, but rather Car()

[Scienfitz]

mostly questions of naming

[AVHopp]

If all open issues are resolved, then this is good to merge