Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Refactor: Parameter Distributions #88
Refactor: Parameter Distributions #88
Changes from 10 commits
b2dc519
2c3d420
6b85f3a
828ef55
10a1614
efef03f
16e6406
04ffee5
8775299
5e324de
f1c4bfe
9a6d7e5
029ff80
cc07d5e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I find the terms real <-> prior could be confusing. For example, I may think of the prior distribution as also being in the real space, since this is the space where my prior belief is based on. What do you think about
transform_bounded_to_unbounded
andtransform_unbounded_to_bounded
?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.
Yeah i mulled over this for a while actually. The terminology came from these 2 perspectives
(i) The modelling assumes we are based in the
Real
space. e.g we think of the parameters arey
inReal
andF: Real -> Data
is the forward model. The Bayesian methodology we developed works only on an unbounded prior space, and so from the mathematical stance we have a single mapG: Prior -> Data
. If we use a mapG
we will need first need a transformationT: Real -> Prior
, then the action isF(y) = G(T(y))
.(ii) The other perspective is that the mathematical stance is
G: Prior -> Data
, and so we define a mapS:Prior -> Real
and then we apply the modelF
, soG(x) = F(S(x))
.From perspective (i) the prior is a e.g lognormal distribution and your forward map is the model, but in order to work with it you need to work in a computational space. From (ii) the prior is a normal distribution, and the forward model maps this to the data (the black box workings don't matter), I preferred the latter because the theory is based on this
Prior
space and not on theReal
space, and this is actually how our code works.So coming to the naming, I don't like mentioning
bounded
because this is only 1 of the 4 cases. But you are right the one key purpose is map to an unbounded space, i do like that; but then is it clear where yourprior
distribution is defined?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 discussed, let's see what the others think. I think if one of the two includes
unbounded
orunconstrained
, there should be no confusion.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.
I agree the naming of
transform_real_to_prior
is a bit confusing. As long as it is explicitly documented in comments and docs it could be fine, since on the surface level a user would probably assume the distribution they are providing is the one in thePrior
space rather than theReal
space.