-
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
Allow constructing log-scaled distributions directly #466
base: development
Are you sure you want to change the base?
Conversation
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.
Overall this looks very good and sensible. I have only one theoretical question: Is there a Jaccobian needed? In other words, do you get the same between those cases:
x1 ~ dnExp(5)
y1 := ln( x1 )
moves.append( mvScale(x1) )
y2 ~ dnLogExponential(5)
moves.append( mvSlide(y2) )
y3 ~ dnLog( dnExp(5) )
moves.append( mvSlide(y3) )
All three should be equivalent, right?
Yes, a Jacobian is included. If we have dnLog(f), then the density is f(log(x))/x. The However, my understanding is that
Since I checked that the density for
The density for I notice that RevBayes does not match R in that RevBayes uses |
Hmm... I notice that the help files for dnLoguniform and dnLogExponential both claim that they are uniform on orders of magnitude. It cannot be true for both of them. I can fix up dnLogExponential so that it has the correct pdf, but then its not clear what its purpose is. So perhaps we should delete it and people can use dnLogUniform instead. Any code that was using dnLogExponential was already not working correctly, I think. |
RevBayesCore::LogExponentialDistribution* d = new RevBayesCore::LogExponentialDistribution( l ); | ||
|
||
return d; | ||
throw RbException()<<"dnLogExponential has been removed from RevBayes.\n * If you want a distribution that is uniform on orders of magnitude, consider dnLoguniform.\n * If you want a distribution whose log is exponential, consider dnLog(dnExponential(lambda))"; |
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.
Why did you remove the distribution? This breaks older scripts that used this distribution. One option could be to internally create LogDistribution if you want to remove some c++ files. The same should apply to the LogUniform distribution?
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.
My reasoning is that there is no option here that is without problems. I think dnLogExponential had the wrong density before. At first I fixed the density, but that silently changes the meaning of the older scripts, while allowing them to run. For example, previously x ~ dnLogExponential(1)
could return a negative x
. However, when the correct density is used, x
is always positive. So it seems like the previous scripts need to be modified somehow. This option is not great because it breaks the older scripts -- but at least it tells the user how to fix them.
I don't like breaking the older scripts, but I also worry about changing their meaning silently. I see that dnLogExponential is used only in one tutorial that was written by Mike May in 2022. We could ask him what he wants to do...
OK, now you can also write:
|
This allows constructing distributions like logNormal by supplying the distribution in log space:
Currently the only log-scaled distributions implemented are dnLognormal, dnLoguniform, and dnLogExponential. This PR allows us to construct (for example)
dnLog(dnGamma(a,b))
,dnLog(dnLaplace(m,s))
,dnLog(dnCauchy(m,s))
, etc.And equally importantly, future distributions that have not been implemented yet.