-
Notifications
You must be signed in to change notification settings - Fork 18
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
Constructors for Gaussian priors with constraints #161
Conversation
Does anyone know how to fix the failing JuliaFormatter/format test? I've run |
91e0b5d
to
dffb677
Compare
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.
Hi Tom - this looks much more along the lines of what I was looking for.
In the todo's, could you also add something to the docs to show how it works?
if ~isapprox(μ_c, m_c, atol = 1e-3, rtol = 1e-2) | ||
@warn "Unable to set constrained mean for `$(name)`: target = $(μ_c), got $(m_c)" | ||
end | ||
if ~isapprox(σ_c, s_c, atol = 1e-3, rtol = 1e-2) |
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.
If these are taken in the constrained space, perhaps they should be normalized? (or is that what rtol does?)
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.
Sorry, I'm lost on the proper context for "normalized" -- normalization, svd. etc. are only done in CES, right?
m_c
and s_c
are the mean/stddev of the distribution at its final, hopefully optimized parameter values, computed as 1d numerical integrals in the physical, constrained space. The highlighted lines are a sanity check to verify that the optimization set those parameters to values that satisfy the user's requested μ_c
, σ_c
for the prior, to very loose tolerances. rtol
is just the tolerance on the relative difference between the two values.
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.
Sorry i was just asking about tolerances. Namely why do we need atol
here? (do both tolerances needed to be satisfied?) Because the constrained space is not normalized so if your domain is on [0,1e-5]
or [0, 1e5]
in the first case atol is satisfied always, and in the second, one could imagine it would never be satisfied
src/ParameterDistributions.jl
Outdated
# optimize in log σ to avoid constraint σ>0 | ||
init_logσ_u = log(init_σ_u) | ||
|
||
# Optimize parameters; could speed up with gradient info but don't bother to. |
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.
If we don't need gradients then they can be removed from the constraints?
Resolution of the climaformat issue mentioned above, written up here for discoverability: @odunbar mentioned that the version of JuliaFormatter had been updated to 0.22; this was in .dev/Project.toml but for some reason .dev/Manifest.toml was still using the older version (I would have thought that that would've been caught and updated when that environment was |
bors try |
tryBuild succeeded: |
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.
As this is user facing - could you also provide an example snippet and some information in the docs priors page?
Otherwise, LGTM!
Finally added docs and examples for |
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.
Sorry it's taken a while for review - this looks great. Please merge after addressing my comments.
- My only real point is that now we have a "recommended" constructor, we should present this first, likewise with the simple example for it, so could you move everything for this up to the top? Then most users will only have to read the first part of this page. After we can talk about general constructors, and all the details for the constraints etc.
Good idea -- I definitely agree that it'll be best for users if we put all the info pertaining to the "recommended" constructor up front. |
@tsj5 Much prefer the structure now! LGTM - merge away! PS - you have been extraordinarily unlucky with the CI failures on your PRs :( |
bors try |
tryBuild succeeded: |
Add Optim, QuadGK as deps Add xform jacobian to Constraint struct Properly handle +/-Inf in Constraints Fix sign of bounded_above() Initial commit of bounded_gaussian Rename bounded_gaussian -> constrained_gaussian Extra tests for codecov Add optional kwargs passed to Optim.optimize Reorder sections of parameter_distributions.md doc Add docs & examples for constrained_gaussian()
bors try |
tryBuild succeeded: |
bors r+ |
161: Constructors for Gaussian priors with constraints r=tsj5 a=tsj5 It's desirable to streamline the UI for specifying parameter priors, in particular those for parameters that live on a constrained domain and hence need a nontrivial Constraint(). This PR does this by adding a new constructor, ~~bounded_gaussian~~ `constrained_gaussian`, for ParameterizedDistribution. The unconstrained distribution is a Gaussian, with parameters (μ_u, σ_u) selected to reproduce user-required statistics in the constrained space [currently only (mean, standard deviation) are supported]. This therefore includes the [lognormal](https://en.wikipedia.org/wiki/Log-normal_distribution) and [logit-normal](https://en.wikipedia.org/wiki/Logit-normal_distribution) distributions as special cases. In the cases where the constraint interval is infinite or semi-infinite, closed-form expressions are used for (μ_u, σ_u). When the interval is finite, no closed-form solution exists, and (μ_u, σ_u) are optimized numerically via the simplex method. This is still less reliable than it should be for large |μ_u| or σ_u: for these cases the distribution becomes concentrated near one or both interval endpoints, causing instabilities for both numerical integration and optimization (since parameter changes have minimal effect on the objective function). This PR fixes a sign error in bounded_above() which caused its Jacobian to be negative. Tasks remaining as of this writing: - constructor for distributions given as Cartesian products of the 1D case currently implemented. - interface for using `constrained_gaussian` to specify priors from parameter file conventions. Co-authored-by: Thomas Jackson <[email protected]>
Build failed: |
bors r+ |
Build succeeded: |
It's desirable to streamline the UI for specifying parameter priors, in particular those for parameters that live on a constrained domain and hence need a nontrivial Constraint().
This PR does this by adding a new constructor,
bounded_gaussianconstrained_gaussian
, for ParameterizedDistribution. The unconstrained distribution is a Gaussian, with parameters (μ_u, σ_u) selected to reproduce user-required statistics in the constrained space [currently only (mean, standard deviation) are supported]. This therefore includes the lognormal and logit-normal distributions as special cases.In the cases where the constraint interval is infinite or semi-infinite, closed-form expressions are used for (μ_u, σ_u). When the interval is finite, no closed-form solution exists, and (μ_u, σ_u) are optimized numerically via the simplex method. This is still less reliable than it should be for large |μ_u| or σ_u: for these cases the distribution becomes concentrated near one or both interval endpoints, causing instabilities for both numerical integration and optimization (since parameter changes have minimal effect on the objective function).
This PR fixes a sign error in bounded_above() which caused its Jacobian to be negative.
Tasks remaining as of this writing:
constrained_gaussian
to specify priors from parameter file conventions.