-
Notifications
You must be signed in to change notification settings - Fork 52
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
Fix obs_noise confusion #400
Conversation
gpjax/variational_families.py
Outdated
A = Lz_inv_Kzx / jnp.sqrt(noise) | ||
A = Lz_inv_Kzx / jnp.sqrt(noise_var) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here do we need the jnp.sqrt
? Could we just use obs_stddev
?
|
||
# A = Lz⁻¹ Kzt / o |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think pre-commit stuff has gone rogue 😱. The comment should has a σ
not an o
. (separate issue).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @henrymoss. This looks really good to me. likelihood.obs_stddev
is fine, but maybe something like e.g., likelihood.root_variance
might read more cleanly. But I leave this up to you. :)
gpjax/objectives.py
Outdated
Sigma = Sigma + cola.ops.I_like(Sigma) * (obs_noise + posterior.prior.jitter) | ||
Sigma = Sigma + cola.ops.I_like(Sigma) * (obs_stddev + posterior.prior.jitter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah @henrymoss this needs to be squared here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks @henrymoss. Just need to fix Dan's comment re. the diagonal noise component and it can be merged.
I have changed all uses of obs_noise (the noise level of our Gaussian likelihood) to obs_stddev which is much clearer.
We were previously inconsistent with this (sometimes it was var and sometimes it was sd) which actually lead to some bugs which have been fixed in this PR.
I have changed a couple of obs_noise=1e-6 to be obs_stddev=1e-3 to keep the same behaviour in our notebooks.