secondmind-labs / trieste

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

`randomize_hyperparameters` ignores `prior_on` #793

Closed j-wilson closed 7 months ago

j-wilson commented 8 months ago

The randomize_hyperparameters helper does not check parameters' prior_on attributes here. Hopefully a quick fix:

if param.prior_on is PriorOn.UNCONSTRAINED:
    param.unconstrained_variable.assign(sample)
else:
    param.assign(sample)
hstojic commented 8 months ago

hi @j-wilson, thanks for spotting this! do you perhaps fancy putting up a PR to fix this? :)

uri-granta commented 8 months ago

Thanks for raising the issue!

As far as I understand, randomize_hyperparameters sets hyperparameters to random samples from the constrained domain if there is a (Sigmoid) constraint, and uses the hyper priors only if there isn't (eg see https://github.com/secondmind-labs/trieste/pull/297). Could you please describe your use case here, so I can better understand how to test this?

j-wilson commented 8 months ago

@uri-granta Sure. Here's a quick example:

import math
import gpflow
import tensorflow_probability as tfp
from trieste.models.gpflow.utils import randomize_hyperparameters

kernel = gpflow.kernels.Matern52()
kernel.variance = gpflow.base.Parameter(
    value=0.1,
    prior=tfp.distributions.Uniform(math.log(0.01), math.log(10.0)),
    prior_on=gpflow.base.PriorOn.UNCONSTRAINED,
    transform=tfp.bijectors.Exp(),
)
randomize_hyperparameters(kernel)

Depending on whether or not the draw from the prior is positive, this code either produces an error

tensorflow.python.framework.errors_impl.InvalidArgumentError: {{function_node __wrapped__CheckNumerics_device_/job:localhost/replica:0/task:0/device:CPU:0}} gpflow.Parameter: the value to be assigned is incompatible with this parameter's transform (the corresponding unconstrained value has NaN or Inf) and hence cannot be assigned. : Tensor had NaN values [Op:CheckNumerics]

or silently assigns an incorrect value to kernel.variance.unconstrained_variable (incorrect because the sample will be mistakenly pulled backward through tfp.bijectors.Exp prior to being assigned).

uri-granta commented 8 months ago

Thanks! Out of curiosity, does anyone know why in the case of sigmoid transforms we prioritise sampling from the constrained domain over using any priors? (cc @vpicheny @hstojic)

uri-granta commented 7 months ago

Closing this issue as it's now been fixed. However, will follow up #794 for automatically extracting parameter bounds and create a separate issue if appropriate.