Skip to content
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 Product to represent independent multivariate distributions #1562

Closed

Conversation

damian-t-p
Copy link

This PR extends Product to build a multivariate distribution out of a list of independent multivariate or univariate distributions.

All of the existing functionality of Product that represents the joint distribution of independent univariate distributions is conceptually unchanged for the joint distribution of independent multivariate distributions.
Functions like mean, cov, pdf etc. can all be assembled out of the independent components in much the same way as in the univariate case.

This PR implements this idea while leaving the previous functionality of Product unchanged.
In particular,

  • the Product type has been extended to allow for a vector of both univariate and multivariate distributions with the same ValueSupport. Distributions with mixed variate types may now have types like Product{Continuous, ContinuousDistribution, Vector{ContinuousDistribution}},
  • all methods previously implemented for Product of univariate distributions have been extended to allow for the multivariate case, and
  • the construction of Product distributions out of multivariate distributions is identical to that out of univariate distributions. In particular, continuous and discrete distributions cannot be mixed,
  • univariate and multivariate distributions can be mixed in the same vector when constructing a product distribution.

Notes

  • The existing tests showed no issues, and tests have been written for the new functionality.
  • Covariance matrices are block-diagonal, and are represented as SparseArrays.SparseMatricCSCs so as not to introduce a BlockDiagonals.jl dependency.
  • There is an inelegance in eltype() --- namely, eltype(ContinuousDistribution) returns Any rather than Float64 (and same in the discrete case), as I believe it should. I've put in a workaround, but let me know if this behaviour is expected.

@codecov-commenter
Copy link

codecov-commenter commented Jun 3, 2022

Codecov Report

Merging #1562 (dcd7978) into master (f889f9e) will increase coverage by 0.05%.
The diff coverage is 100.00%.

❗ Current head dcd7978 differs from pull request most recent head b7a428b. Consider uploading reports for the commit b7a428b to get more accurate results

@@            Coverage Diff             @@
##           master    #1562      +/-   ##
==========================================
+ Coverage   85.52%   85.58%   +0.05%     
==========================================
  Files         128      128              
  Lines        7863     7888      +25     
==========================================
+ Hits         6725     6751      +26     
+ Misses       1138     1137       -1     
Impacted Files Coverage Δ
src/Distributions.jl 100.00% <ø> (ø)
src/multivariate/product.jl 100.00% <100.00%> (ø)
src/multivariate/multinomial.jl 86.39% <0.00%> (+0.68%) ⬆️

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 f889f9e...b7a428b. Read the comment docs.

_mappartitioned replaces _partitionargs to work better with FillArrays
and Tracker.
@devmotion
Copy link
Member

Thanks for the PR!

I think we should generalize Product (see #1391), going beyond what is currently implemented in DistributionsAD and used in Turing. However, I think it would be more consistent, both with the univariate case and existing use of product distributions in Turing etc., to not follow the approach in this PR here but make a product distribution of a vector of multivariate distributions a matrix-variate distribution (and a matrix of univariate distributions a matrix-variate distribution, a matrix of multivariate distributions a distribution of three-dimensional arrays etc.).

IIRC #1391 is ready and there's nothing major left to discuss. Unfortunately, IIRC it is not merged yet since it contains a breaking change - and we are very cautious with breaking releases since people don't like them it seems (my personal view is that this policy delays some major improvements and nobody should feel pressed to jump to the next minor or major release immediately anyway).

@damian-t-p
Copy link
Author

Thanks for the link to the existing PR (I guess I should have done a more thorough check of feature proposals)! It looks very promising, and contains exactly what I need, so I will be sure to watch it.

@damian-t-p damian-t-p closed this Jun 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants