search

PEtab-dev / PEtab

PEtab - an SBML and TSV based data format for parameter estimation problems in systems biology
https://petab.readthedocs.io
MIT License
56 stars 12 forks source link

PriorParameters in sampling are not transformed for parameterScaleUniform #491

Closed jvanhoefer closed 3 years ago

jvanhoefer commented 3 years ago

In the sampling routines for ParameterScaleUniform, the lower/upper bounds are not transformed to the parameter scale. (Clipping then might lead to samples mostly on one bound...).

jvanhoefer commented 3 years ago

@dweindl @yannikschaelte I am not to sure, if the other two parameterScale distributions are correct either, but I am not sure, if I understand the meaning of their parameters correctly. Is the mean of the (log)-normal there given in parameter scale or in linear scale?

jvanhoefer commented 3 years ago

Same for parameterScaleUniform: I had in mind, that prior-parameters are always specified in linear scale, and hence parameterScaleUniform would still take the bounds in linear scale (hence changing the scale would not change the support of the distribution, only the distribution would not be skewed to one side e.g. in log space...). Currently the tests are not written in such way and my PR in #492 is according to the tests (which are not how I would have understood all of these from the docs...)

yannikschaelte commented 3 years ago

In the documentation it should be stated that for the parameterScaleX type priors, values are on the parameter scale (original), not linear.

yannikschaelte commented 3 years ago

... which is apparently not clearly the case, thanks for pointing out ... @dweindl do you remember? were parameterScale parameters supposed to be on or off scale?

jvanhoefer commented 3 years ago

That on the other hand would mean, that for the default (parameterScaleUniform) you have to change the prior parameters, when changing the scale? (Or put the other way: Everything is always in linear scale, but parameters for parameterScaleX). I find this confusing, but ok... If this is the case, my PR is "correct".

yannikschaelte commented 3 years ago

I think the motivation here was that in terms of distributions, one wants to specify the distribution parameters on the scale where they are applied. E.g. for a normal N(0,1) one does not want 1;10. But both goes I think.

dweindl commented 3 years ago

My understanding was that everything should be on linear scale, so that any change of parameterScale would not directly require a change in the other fields. However, for parameterScaleX requiring linear scale would have the opposite effect.

I am not sure whether there really has been a deliberate decision.

Agreed that docs need clarification.