Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

randomize_hyperparameters ignores prior_on #793

Closed
j-wilson opened this issue Nov 2, 2023 · 5 comments · May be fixed by #794
Closed

randomize_hyperparameters ignores prior_on #793

j-wilson opened this issue Nov 2, 2023 · 5 comments · May be fixed by #794
Labels
bug Something isn't working

Comments

@j-wilson
Copy link

j-wilson commented Nov 2, 2023

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 hstojic added the bug Something isn't working label Nov 17, 2023
@hstojic
Copy link
Collaborator

hstojic commented Nov 17, 2023

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

@j-wilson j-wilson mentioned this issue Nov 17, 2023
3 tasks
@uri-granta
Copy link
Collaborator

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 #297). Could you please describe your use case here, so I can better understand how to test this?

@j-wilson
Copy link
Author

@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
Copy link
Collaborator

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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants