Fix SampleFromUniform and implement AbstractMCMC interface for SampleFromPrior and SampleFromUniform

[devmotion]

This PR enables sampling from the prior and with the uniform initialization.

Unfortunately, I have no idea what distributions we expect with the uniform initialization - I studied the code in utils.jl, but I'm a bit confused why we don't just sample from the normal and inverse gamma prior - shouldn't only TransformDistribution, SimplexDistribution, and PDMatDistribution be initialized with a different algorithm? So the test for the uniform initialization has to be adjusted to the correct expected values.

[mohamed82008]

I don't think SampleFromUniform is applicable here. It is only used for initializing HMC.

[mohamed82008]

It's also buggy. I will make a PR to fix it.

[codecov]

Codecov Report

Merging #77 into master will increase coverage by 0.09%.
The diff coverage is 42.30%.

@@            Coverage Diff             @@
##           master      #77      +/-   ##
==========================================
+ Coverage   76.92%   77.01%   +0.09%     
==========================================
  Files          13       13              
  Lines         845      844       -1     
==========================================
  Hits          650      650              
+ Misses        195      194       -1     

Impacted Files	Coverage Δ
src/DynamicPPL.jl	100.00% <ø> (ø)
src/utils.jl	53.19% <0.00%> (+0.13%)	⬆️
src/context_implementations.jl	56.02% <40.00%> (-0.44%)	⬇️
src/sampler.jl	58.82% <71.42%> (+8.82%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 26d90d7...8090bdd. Read the comment docs.

[cpfiffer]

I don't think SampleFromUniform is applicable here. It is only used for initializing HMC.

I think it's worth including. I'm using it a lot in the MLE/MAP code so I would like to see SampleFromPrior and SampleFromUniform be treated similarly.

[mohamed82008]

I think we should store a VarInfo(model) inside the SampleFromPrior struct to use the TypedVarInfo.

[devmotion]

I'm not sure if it's worth constructing a TypedVarInfo object from the UntypedVarInfo object in every sample step. If you use Turing, by default a Chains object will be constructed from the vector of UntypedVarInfo anyways, and otherwise you can still convert the vector to a vector of TypedVarInfo afterwards if needed.

[devmotion]

This PR reverts parts of #78. The problems with this PR are:

• It just set the trans flag to true for every variable even though not for all variables a transformation exists.
• The initialization in the transformed space with uniform random numbers in the range between -2 and 2 is only performed for const Transformable = Union{TransformDistribution,SimplexDistribution,PDMatDistribution}, which does not include PositiveDistribution and UnitDistribution that are added in this PR. In particular, that implies that for the InverseGamma distribution samples are generated from the prior over the positive real numbers even though the trans flag is set to true (as shown in the tests of the linked PR and the tests of this PR).
• Just including the PositiveDistribution among the list of transformable distributions is not sufficient either. Since the implementation of inittrans transforms the samples back to the original space (i.e., the positive real numbers), actually the untransformed variables are saved. However, due to the trans flag being set to true, any indexing operation will perform the transformation to the original space once more (i.e., perform a second exponentiation step in the case of the InverseGamma distribution), which leads to a distribution of samples that are not initialized from the uniform distribution in the transformed space. Moreover, in the assume implementations one has to return the untransformed values anyways, since otherwise the next step in the model evaluation in which sqrt(s) is computed (with s being the InverseGamma distributed random variable) will fail if s is negative.
• The PR leads to inconsistent behaviour of univariate and multivariate distributions since it only changed the behaviour of univariate distributions.

This PR now reverts the changes related to the trans flag and the transformed spaces and extends the behaviour of always replacing samples from SampleFromUniform to multivariate distribution. The tests yield the expected results, the behaviour of univariate and multivariate distributions is consistent, the trans flag corresponds to the the actual space of the samples (and hence also indexing yields the expected values), and both distributions that can be transformed (now also including distributions on the positive real line and the unit interval) and distributions for which no transformation is defined are handled correctly.

[mohamed82008]

Looks good. Thanks @devmotion! Btw, UnitDistribution and PositiveDistribution are subtypes of TransformDistribution so adding them to the Union is not necessary.

[devmotion]

PositiveDistribution is just a Union, isn't it? https://github.com/TuringLang/Bijectors.jl/blob/cd349b7cb6d2dc3b7109bb8492f6c54f335eef2b/src/Bijectors.jl#L224-226

InverseGamma distributions are definitely not covered by the current implementation (and hence just sampled from the prior) - I spent so much time with debugging that I'm quite sure about this point 😄

[devmotion]

And UnitDistribution as well: https://github.com/TuringLang/Bijectors.jl/blob/cd349b7cb6d2dc3b7109bb8492f6c54f335eef2b/src/Bijectors.jl#L240

[mohamed82008]

Yes both are unions but TransformDistribution is the biggest union of them all :)

julia> InverseGamma <: DynamicPPL.TransformDistribution
true

[mohamed82008]

I think the reason why it is sampled from the prior is that invlink is always called so setting trans to true was wrong in #78. But this is true for all distributions it seems. So thanks for fixing this bug :)

[devmotion]

Hmm...😄 Not sure why I thought it didn't work. But is it also a subtype of DynamicPPL.Transformable? 🤔

[mohamed82008]

But is it also a subtype of DynamicPPL.Transformable?

Yes