Botorch preset#757
Conversation
a149bb3 to
c3885f6
Compare
c3885f6 to
4f4fd55
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new BOTORCH Gaussian Process preset intended to exactly mirror BoTorch GP behavior (single- and multi-task), alongside supporting kernel-factory refactors and validation improvements.
Changes:
- Introduce
GaussianProcessPreset.BOTORCHand implement BoTorch-aligned kernel / mean / likelihood factories (including custom GPyTorch components for multitask mean + likelihood). - Add parameter-kind classification (
ParameterKind) and automatic parameter-kind validation for kernel factories via a new_KernelFactorybase. - Extend tests (BoTorch parity + kernel-factory validation) and update the Streamlit surrogate-model demo to expose GP presets and multitask/transfer-learning toggles.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
baybe/surrogates/gaussian_process/presets/botorch.py |
Adds BoTorch preset factories for kernel/mean/likelihood. |
baybe/surrogates/gaussian_process/components/_gpytorch.py |
Introduces custom GPyTorch mean + multitask likelihood helper to match BoTorch behavior. |
baybe/surrogates/gaussian_process/components/kernel.py |
Adds _KernelFactory w/ parameter-kind validation; adjusts ICMKernelFactory. |
baybe/surrogates/gaussian_process/presets/{edbo,edbo_smoothed}.py |
Migrates kernel factories to _KernelFactory and updates dimension logic. |
baybe/surrogates/gaussian_process/presets/{core,__init__.py} |
Adds BOTORCH enum entry and re-exports preset factories. |
baybe/parameters/{enum,base.py} |
Adds ParameterKind and exposes Parameter.kind. |
baybe/parameters/selectors.py |
Removes _ParameterSelectorMixin (superseded by _KernelFactory). |
tests/test_gp.py |
Adds parity test asserting BOTORCH preset reproduces BoTorch posterior stats. |
tests/test_kernel_factories.py |
Adds tests for kernel-factory parameter-kind validation. |
streamlit/surrogate_models.py |
Adds GP preset selector + multitask / transfer-learning UI and logic. |
docs/conf.py |
Updates nitpick ignore list for refactor. |
CHANGELOG.md |
Documents new presets and kernel-factory parameter-kind validation. |
Comments suppressed due to low confidence (1)
baybe/surrogates/gaussian_process/presets/edbo.py:85
effective_dimsis computed astrain_x.shape[-1], which includes the task feature column in transfer-learning searchspaces. Since this factory explicitly excludesTaskParameterviaparameter_selector, the priors should likely be based on the non-task dimensionality (i.e. exclude the task feature) to avoid skewing the dimension-dependent prior heuristics. Consider derivingeffective_dimsfrom the selected non-task dimensions instead (e.g., subtract task dims or count comp-rep columns for selected parameters).
effective_dims = train_x.shape[-1]
switching_condition = _contains_encoding(
searchspace.discrete, _EDBO_ENCODINGS
) and (effective_dims >= 50)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
680d8a1 to
eeaa63d
Compare
e409228 to
eeaa63d
Compare
AVHopp
left a comment
There was a problem hiding this comment.
The streamlit changes should be a separate PR as they have nothing to do with new presets. I do see the value in extending the streamlit but those two things should still not be combined imo.
eeaa63d to
7bb590b
Compare
AVHopp
left a comment
There was a problem hiding this comment.
Two bugs or uncatched errors in the streamlit
Scienfitz
left a comment
There was a problem hiding this comment.
sneaky global change to mll is not acceptable
7ac4c25 to
9b90df2
Compare
71b8c2b to
5c1f0c9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
baybe/surrogates/gaussian_process/core.py:193
fit_criterion_factorychanged its init alias fromcriterion_or_factorytofit_criterion_or_factory, which is a breaking change for any callers still using the old keyword. If this is intended, consider keepingcriterion_or_factoryas a deprecated alias (or otherwise providing a compatibility shim) so existing user code/configs don’t fail on upgrade.
fit_criterion_factory: FitCriterionFactoryProtocol = field(
alias="fit_criterion_or_factory",
factory=BayBEFitCriterionFactory,
converter=partial( # type: ignore[misc]
to_component_factory, component_type=GPComponentType.CRITERION
),
validator=is_callable(),
)
| set(d.tolist()) if (d := task_kernel.active_dims) is not None else all_idcs | ||
| ) | ||
|
|
||
| if not base_idcs <= allowed_base_idcs: |
There was a problem hiding this comment.
Why is it allowed that they are a subset? (also for task idxs)
There was a problem hiding this comment.
A a user, when I explicitly pass a kernel instead of relying to the defaults, I can restrict that kernel to any active dimensions. As long as the numeric part references any subset of the numeric parameters, I'm fine. Same for the task kernel. But the factory has no direct control over this, so all we can do is validate. Makes sense?
There was a problem hiding this comment.
makes sense. I think I was thinking for some reason this is checking the "overall" / "final" kernel. There we would expect that each comp column has a kernel associated, right?
| MEAN_FACTORY = BotorchMeanFactory() | ||
| LIKELIHOOD_FACTORY = BotorchLikelihoodFactory() | ||
| # Botorch dictates no specific criterion, so we fill the preset with our default | ||
| FIT_CRITERION_FACTORY = BayBEFitCriterionFactory() |
There was a problem hiding this comment.
arguing that the fit_gpytorch_mll offers a generic interface and hence has no default is omitting that botorch uses MLL for all its examples and code - probably also in the paper. I think its fair to say the BOTORCH fit criterion is MLL
There was a problem hiding this comment.
yeah sure, but what do you expect me to do? remove the comment? Can do, if you prefer
There was a problem hiding this comment.
seems I need to rephrase my comment: botorch uses the MLL in its examples, also for TL. Subsequently your assignment of the baybe factory for the likelihood is not correct as this would be only correct if we had no evidence of what botorch uses for TL
|
|
||
| active_settings.random_seed = 1337 | ||
| gp = GaussianProcessSurrogate.from_preset( | ||
| "BOTORCH", fit_criterion_or_factory=FitCriterion.MARGINAL_LOG_LIKELIHOOD |
There was a problem hiding this comment.
pending discussion: should MLL be set by the botorch preset?
There was a problem hiding this comment.
MLL is set by the botorch preset – for the single-task case. The question is rather: how do want the multi-task case to execute and/or do we need to adjust what botorch sets for this case? From the test perspective, everything works as long as the botorch landing page code (which is executed by the test) uses the same criterion as the preset. But we can change either of the two
Now properly handles the regular/task parameter split
baa28f7 to
7996e1e
Compare
DevPR, parent is #745
Adds the
BOTORCHpreset for GPs.Important information
this is the BoTorch behaviorseems like a bad idea. The test ensures this explicitly.