Fix Wishart and InverseWishart

[devmotion]

No description provided.

[mohamed82008]

So I haven't really checked the changes closely next to Distributions.jl but this PR seems to pretty much just rename -c0 to logc0. I don't quite understand how that makes sense from a mathematical point of view. Are you sure this is correct?

[devmotion]

Yes, and a fix of the variance computation, it was incorrect before. The test error with Zygote is due to the @warn in https://github.com/JuliaStats/Distributions.jl/blob/20c91d9efcc5f96913bf8e38be2e3fb14b21942b/src/matrix/wishart.jl#L53. Needs a custom adjoint I guess.

[devmotion]

I don't quite understand how that makes sense from a mathematical point of view.

I guess the name logc0 is more descriptive, since it's the logarithm of the scalar multiplicative factor. Mathematically it just doesn't matter if you use the variable for -log(c0) or log(c0), where c0 is the previous variable 🤷‍♂️ I guess one motivation might be that you then avoid the subtractions in the computations of the logpdf.

[johnczito]

So I haven't really checked the changes closely next to Distributions.jl but this PR seems to pretty much just rename -c0 to logc0. I don't quite understand how that makes sense from a mathematical point of view. Are you sure this is correct?

Sorry! I suppose I'm to blame for these changes. A density is kernel x normalizing constant, which you can write either as (i) f(x) = c₀⋅k(x) or (ii) f(x) = k(x) / Z₀. Doesn't matter which you pick as long as you pick one. The 5 other matrix-variate distributions went with (i) but the Wisharts went with (ii). I just harmonized it so that there is a single method _logpdf(d, X) = logkernel(d, X) + d.logc0 for all of them.

While we're at it (and re this), I am curious why these distributions have to be line-for-line recoded here. Is it the use of PDMats? Where is Distributions dropping the ball?

[devmotion]

While we're at it (and re this), I am curious why these distributions have to be line-for-line recoded here. Is it the use of PDMats

I haven't implemented the version in DistributionsAD (just trying to fix it), but yes, it seems, that the major difference is that Turing(Inverse)Wishart doesn't work with AbstractPDMats. Otherwise it's mainly minor differences such as additional calls of float when converting the constants (not sure why that's needed though).

[mohamed82008]

For Zygote, we need to avoid stuff that break Zygote like try-catch, so we re-implement the function avoiding those things that break Zygote. PDMats also causes issues when trying to differentiate wrt the random variable for almost all AD backends. So we have TuringX distributions which avoid PDMats. This is also true for TuringDenseMvNormal.

[devmotion]

CI failure seems to be unrelated, the test was cancelled without any test failures.

[mohamed82008]

Thanks @devmotion !

[devmotion]

Is it OK to make a new release with this fix (it seems it should be a breaking 0.6 release)? Then we could remove all these temporary fixes in the downstream packages.

[mohamed82008]

Sure, sounds good.

[devmotion]

Thanks!