Skip to content
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

PriorParameters in sampling are not transformed for parameterScaleUniform #491

Closed
jvanhoefer opened this issue Dec 14, 2020 · 7 comments
Closed

Comments

@jvanhoefer
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Member

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

@yannikschaelte
Copy link
Member

... 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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

dweindl commented Dec 15, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants