secondmind-labs / trieste

A Bayesian optimization toolbox built on TensorFlow
Apache License 2.0
221 stars 42 forks source link

Allow SeparateIndependent kernels in gpflow decoupled sampler #736

Closed khurram-ghani closed 1 year ago

khurram-ghani commented 1 year ago

Addresses #724.

This PR adds SeparateIndependent kernels support to DecoupledTrajectorySampler for gpflow models. As part of updating the tests, I have removed some of the duplication in the sampler tests.

Similar support for RandomFourierFeatureTrajectorySampler can be added in a follow-on PR. Also, perhaps there is potential for combining this decoupled trajectory sampler with the one for gpflux DeepGaussianProcessDecoupledTrajectorySampler and simplifying. This could be covered in another PR.

khurram-ghani commented 1 year ago

Looks great, I couldn't fully track what was being tested with those changes, just to recap it's really important to check that:

  • trajectory samplers return samples from the posterior distribution for a variety of models
  • trajectory samplers update when needed
  • and I guess the shapes of weights and samples are consistent for all models.

@vpicheny We have tests for these, covering these models:

I am pretty sure that the current tests cover all of this (including the integration tests with Thompson Sampling I imagine?) but it wouldn't hurt to double-check

I found this integration test with Thompson sampling that covers this. It tests single output only though. Do we need a multioutput test here? Do we have existing toy problems for multioutputs?

vpicheny commented 1 year ago

I found this integration test with Thompson sampling that covers this. It tests single output only though. Do we need a multioutput test here? Do we have existing toy problems for multioutputs?

I don't think that we can get a multi-output integration test easily. The current integration test will check that we haven't introduced a bug for the single-gp case, which is already quite nice I think! All in all it looks like the test coverage is good enough as it is, thanks a lot for checking.