search

beast-dev / beast-mcmc

Bayesian Evolutionary Analysis Sampling Trees
http://beast.community
GNU Lesser General Public License v2.1
191 stars 72 forks source link

ScaleOperator error on proportion of invariant sites #955

Closed GuyBaele closed 6 years ago

GuyBaele commented 7 years ago

Generate a BEAST XML in BEAUti and select to assume a proportion of invariant sites.

Upon running in BEAST, I get the following error: SEVERE: Parsing error - poorly formed BEAST file, infile.xml: Error parsing '' element with id, 'null': Scale operator can't be used on parameters with a finite upper bound (use a RandomWalk) (pInv)

Error thrown at: dr.inferencexml.operators.ScaleOperatorParser.parseXMLObject(ScaleOperatorParser.java:75)

The XML contains the following:

Removing the upper="1.0" attribute from the XML does not fix the problem though. It seems as if the uniform prior (in the prior block), which has a correct finite upper bound, is what prevents this from running?

GuyBaele commented 7 years ago

Same issue with birthDeath.relativeDeathRate.

rambaut commented 7 years ago

These are [0, 1] parameters and should be a random walk (possibly on a logit scale?). On the other hand I would really love to drop pInv all together.

GuyBaele commented 7 years ago

I don't really mind dropping the pInv, but the general principle still holds: for some models, we may need a probability parameter in the [0,1] space and we'll have to estimate it using a correct transition kernel. Random walk after logit seems fine to me.

rambaut commented 7 years ago

Absolutely agree. Actually one of the reasons I put the restriction on scale with an upper bound is for that reason. If we think the posterior will be diffuse over the 0,1 interval (i.e., sampling a tipdate within a year) then a uniform may be appropriate. With pInv a tuneable random walk is the way to go.

rambaut commented 7 years ago

Didn't mean to close this.

GuyBaele commented 7 years ago

Should probably create a novel transition kernel, which first performs a logit transformation, then a random walk on the transformed parameter and then does an inverse logit. Need to take into account the Jacobian for the logit transformation. We can keep the U[0,1] prior on the original variable.

rambaut commented 7 years ago

Probably easier to have a reflected random walk on these and then use a beta distribution prior.

rambaut commented 7 years ago

Was just thinking about a RW Logit kernel. Could be useful.

rambaut commented 6 years ago

To follow this up, randomWalkOperator can take boundaryCondition="logit". Will make this the default TK on pInv. Probably should change the name of the attribute to transform="logit" (or log).