Distributions.jl icon indicating copy to clipboard operation
Distributions.jl copied to clipboard

Remove constraint on AbstractPDMat

Open oxinabox opened this issue 3 years ago • 4 comments

Closes #1219 Tests it works by using BlockDiagonals.jl

Basic philosophy:

  • Anything that worked with PDMats before should be just as fast
  • Anything that didn't work before should be correct, but might be slower
    • (e.g. we might recompute the cholesky everytime rand is called)

TODO

  • [x] logpdf
  • [x] rand
  • [x] sqmahal(d::MvNormal, x::AbstractVector)
  • [x] sqmahal(d::MvNormal, x::AbstractMatrix)
  • [x] MvNormalCanon
  • [x] https://github.com/JuliaStats/PDMats.jl/pull/162 needs to be merged and tagged

oxinabox avatar May 18 '22 19:05 oxinabox

I think it would be great if it would be possible to use different matrices and types as covariance matrices.

I'm not happy about the added complexity in Distributions though and think there should be a better way to handle this. In particular, I think that similar generalizations are also desirable in other scenarios where currently AbstractPDMats are required.

Maybe we could instead add these fallbacks of invquad, invquad!, unwhiten! etc. for AbstractMatrix to PDMats instead? And then only loosen the restrictions in the types of e.g. MvNormal in Distributions?

devmotion avatar May 18 '22 20:05 devmotion

I think that would make sense. It also would make it easier for other matrix packages to overload them.

One issue is I have no interest in implementing all of them, just like these 3, which is all we need for MvNormal.

@andreasnoack what do you think. Would such a PR be welcomed to PDMats.jl?

I would still want to add these test types here.


One thing I have shown here right now is the code changes are not hard or deep. Just a few operations. This is much easier than creating the work arounds.

oxinabox avatar May 18 '22 20:05 oxinabox

This should now be good to go.

oxinabox avatar May 31 '22 19:05 oxinabox

I started the registration process for PDMats 0.11.11: https://github.com/JuliaRegistries/General/pull/61420 Let's rerun tests when it's available.

devmotion avatar May 31 '22 20:05 devmotion