Add new parameter prior distributions #595
Conversation
dilpath
left a comment
dilpath
left a comment
There was a problem hiding this comment.
Happy to review again before merging.
I would keep this as a draft until it is decided whether
parameterScaleshould remain part of the PEtab standard
👍
a mathematical definition is provided for each prior
👍
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
matthiaskoenig
left a comment
matthiaskoenig
left a comment
There was a problem hiding this comment.
Some minor comments
dweindl
left a comment
dweindl
left a comment
There was a problem hiding this comment.
Great, thanks!
I would keep this as a draft until it is decided whether parameterScale should remain part of the PEtab standard.
Agreed.
|
Following the reorganization of the parameter table, I have now updated the prior specification. Regarding truncated priors, this PR proposes that the parameter bounds define the truncation points. |
dweindl
left a comment
dweindl
left a comment
There was a problem hiding this comment.
Thanks, looks good to me.
Renaming prior to priorDistribution (#618 (comment)) could be included here, but can also be done separately.
doc/v2/documentation_data_format.rst
Outdated
| Prior parameters used for the :ref:`MAP objective function and for Bayesian inference <v2_objective_function>`. | ||
| Prior parameters used for the | ||
| :ref:`MAP objective function and for Bayesian inference <v2_objective_function>`. | ||
| ``priorParameters`` is required if ``prior`` is non-empty. |
There was a problem hiding this comment.
For uniform we could allow empty parameters instead of repeating the bounds?
On second thought, why not just leave the prior distribution empty in that case...
There was a problem hiding this comment.
Fair point. I lean more towards that we should maintain consistency that if a priorDistribution is specified, then parameters must be specified.
Good point, I will add it here. |
Co-authored-by: Daniel Weindl <dweindl@users.noreply.github.com>
Co-authored-by: Daniel Weindl <dweindl@users.noreply.github.com>
sebapersson
changed the title
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
doc/v2/documentation_data_format.rst
Outdated
| - *logUniform*: parameters of corresponding uniform distribution (see uniform) | ||
| - *normal*: mean; standard deviation (**not** variance) | ||
| - *rayleigh*: scale | ||
| - *uniform*: lower bound; upper bound |
There was a problem hiding this comment.
do we really need extra lower/upper bounds? shouldn't that be implied by parameter bounds?
There was a problem hiding this comment.
This is a good, but also tricky point. You are correct the lower- and upper parameter bounds do imply the bounds. However, I think for consistency that we should for all priors enforce that parameters are specified. I am open for discussing this though.
|
Following review suggestions I added the priors in a table instead using the |
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
|
This can be merged now right? |
Add additional probability distributions as required for PEtab-dev/PEtab#595. See PEtab-dev#374.
Add additional probability distributions as required for PEtab-dev/PEtab#595. See PEtab-dev#374.
Add additional probability distributions as required for PEtab-dev/PEtab#595. See PEtab-dev#374.
Add additional probability distributions as required for PEtab-dev/PEtab#595. See #374.
5 participants
I have added information for the new
initializationPriorTypeandobjectivePriorTypedistributions, along with support for truncated distributions in the PEtab v2 specification. Additionally, a mathematical definition is provided for each prior, since some distributions, such as the gamma distribution, can be parameterized in different ways.I would keep this as a draft until it is decided whether
parameterScaleshould remain part of the PEtab standard. IfparameterScaleis retained, then I believe we should also keep theparameterScalepriors where it makes sense. This is because inference is sometimes performed directly onlog(p)(the logarithm of parameterp), as the geometry of the posterior landscape is often more favorable, and to avoid issues with transforming prior distributions, users sometimes place a prior on the log of the parameter directly—even though a prior on the log scale can be nonintuitive (for example, a uniform prior on the log scale is not uniform on the linear scale :).I will also update the test-suite to test for correct prior evaluation on
objectivePriorTypefor the linear priors (including testing truncated priors).