Generalize `Product` #1391

devmotion · 2021-08-31T19:53:42Z

DistributionsAD contains custom distribution types for vectors of multivariate distributions and matrices of univariate distributions. I think it would be good to add support for such more general product distributions to Distributions and remove them from DistributionsAD.

In this PR, I propose to generalize the existing Product such that it allows to construct M + N-dimensional product distributions from AbstractArray{<:Distribution{ArrayLikeVariate{M}},N} by stacking the M-dimensional distributions. The ArrayLikeVariate variate form (thanks to @oschulz!) enables us nicely to define these product distributions in an even more general way than what currently is implemented in DistributionsAD.

I tried to perform this generalization in a non-breaking way by adding a new type ProductDistribution, making Product a special alias of it, and deprecating the Product constructor. I did not find a way to deprecate Product with Base.@deprecate_binding, it seems it can't deal with type parameters: JuliaLang/julia#9830. Product still exists and product_distribution(::AbstractVector{<:UnivariateDistribution}) still creates a Product to avoid breaking changes (otherwise e.g. the downstream tests fail).

This PR requires:

implementation of interface for MatrixOfUnivariateDistribution
implementation of interface for VectorOfMultivariateDistribution
basic methods for more general cases
tests
implementations of more efficient dispatches for product distributions of Fill{<:Distribution{<:ArrayLikeVariate}} (if necessary; not all implementations in DistributionsAD might be needed)

cc: @yebai

oschulz · 2021-08-31T20:17:15Z

I like the ability to create M + N products!

Question: Shouldn't we also allow for products of multivaritate dists (of same length) that result in matrixvariate dists, and so on for higher orders?

devmotion · 2021-08-31T20:17:48Z

Question: Shouldn't we also allow for products of multivaritate dists (of same length) that result in matrixvariate dists, and so on for higher orders?

That's exactly what the PR proposes to do 🙂 And it is done in VectorOfMultivariate in DistributionsAD with vectors of multivariate distributions.

oschulz · 2021-08-31T20:19:21Z

Oh, silly me! Sorry for the misunderstanding - now I like it even more! :-)

codecov-commenter · 2021-08-31T20:42:00Z

Codecov Report

Merging #1391 (55e6c5b) into master (f889f9e) will decrease coverage by 0.94%.
The diff coverage is 98.75%.

❗ Current head 55e6c5b differs from pull request most recent head 1a574fa. Consider uploading reports for the commit 1a574fa to get more accurate results

@@            Coverage Diff             @@
##           master    #1391      +/-   ##
==========================================
- Coverage   85.52%   84.57%   -0.95%     
==========================================
  Files         128      125       -3     
  Lines        7863     7140     -723     
==========================================
- Hits         6725     6039     -686     
+ Misses       1138     1101      -37

Impacted Files	Coverage Δ
src/common.jl	`79.12% <0.00%> (-0.68%)`	⬇️
src/multivariates.jl	`40.90% <ø> (-1.95%)`	⬇️
src/multivariate/product.jl	`100.00% <100.00%> (ø)`
src/product.jl	`100.00% <100.00%> (ø)`
src/univariate/continuous/studentizedrange.jl	`72.72% <0.00%> (-11.89%)`	⬇️
src/univariate/continuous/noncentralchisq.jl	`78.94% <0.00%> (-7.42%)`	⬇️
src/univariate/continuous/chisq.jl	`75.00% <0.00%> (-6.82%)`	⬇️
src/univariate/continuous/pgeneralizedgaussian.jl	`61.76% <0.00%> (-6.81%)`	⬇️
src/univariate/continuous/noncentralf.jl	`85.00% <0.00%> (-6.67%)`	⬇️
src/univariate/continuous/noncentralt.jl	`85.00% <0.00%> (-5.91%)`	⬇️
... and 106 more

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...1a574fa. Read the comment docs.

matbesancon · 2021-10-05T08:11:20Z

like the idea as well

oschulz · 2021-11-07T13:41:38Z

Any news on this one?

devmotion · 2021-11-07T14:08:19Z

I have quite many changes in a local branch but haven't had time to clean and polish them. There were some test errors caused by bugs in upstream packages such as StatsBase, so I had to fix these first. I'll try to finish the PR this week.

mschauer · 2021-11-14T11:41:20Z

How do you treat the value support? Force all factors to have the same value support?

oschulz · 2021-11-14T23:00:31Z

Thanks for doing this, @devmotion , this will come in so handy!

devmotion · 2021-11-27T23:22:08Z

Sure, this could be done (and I've thought of it - with a different notation) but I thought such convenience functions could be discussed and possibly added in a separate PR as it is not needed for this PR. Hence I don't really want to start a discussion here but just mention that I thought \otimes (ie tensor product) would be the natural choice.

cscherrer · 2021-11-27T23:23:41Z

Ideally, yes, but in LinearAlgebra, × === cross. See e.g.
https://julialang.zulipchat.com/#narrow/stream/225540-gripes/topic/.5Ctimes
and
JuliaLang/julia#37109

It's unfortunate, because the symbol is very common in other contexts. But at least for now, IMO this is reason enough to avoid it when possible.

oschulz · 2021-11-28T11:36:57Z

Sure, this could be done (and I've thought of it - with a different notation) but I thought such convenience functions could be discussed and possibly added in a separate PR as it is not needed for this PR.

Sure, of course.

Hence I don't really want to start a discussion here but just mention that I thought \otimes (ie tensor product) would be the natural choice.

Oh yes, that would be neat too, and avoid the × === cross awkwardness.

oschulz · 2021-11-28T11:38:09Z

@cscherrer, maybe you can start playing with \otimes in MeasureBase to see how that "feels"?

cscherrer · 2021-11-28T12:59:17Z

I think we'll need to step back and look at the notation as a whole, hopefully get something self-consistent that doesn't conflict too badly with Distributions. But let's discuss this elsewhere 🙂

devmotion · 2021-11-30T22:59:57Z

Principally, this PR is ready for review - but I really would like to revert the last commit which fixed the downstream tests. It introduces an annoying inconsistency, namely that product_distribution(::AbstractVector{<:UnivariateDistribution}) falls back to creating a Product whereas for all other inputs the default definition of product_distribution creates a ProductDistribution. The ProductDistribution is clearly superior, it supports everything that Product does and fixes some of its bugs (such as support of FillArrays). However, it seems that Broadcast.broadcased expressions (used instead of zip to benefit from pairwise summation) are problematic for AD.

mschauer · 2022-01-18T09:38:31Z

src/product.jl

+independent `M`-dimensional distributions by stacking them.
+
+The function falls back to constructing a [`ProductDistribution`](@ref) distribution but
+specialized methods can be defined.


Should we say here what pdf(d::ProductDistribution, x) does?

oschulz · 2022-06-03T09:33:57Z

This is going to be awesome, thanks so much for this @devmotion !

devmotion · 2022-07-07T00:07:38Z

Principally, this PR is ready for review - but I really would like to revert the last commit which fixed the downstream tests. It introduces an annoying inconsistency, namely that product_distribution(::AbstractVector{<:UnivariateDistribution}) falls back to creating a Product whereas for all other inputs the default definition of product_distribution creates a ProductDistribution. The ProductDistribution is clearly superior, it supports everything that Product does and fixes some of its bugs (such as support of FillArrays). However, it seems that Broadcast.broadcased expressions (used instead of zip to benefit from pairwise summation) are problematic for AD.

Any comments? In principle the PR seems ready.

oschulz · 2022-07-07T08:09:17Z

Any comments? In principle the PR seems ready.

Just that I can't wait to use it! :-)

matbesancon · 2022-07-07T11:10:45Z

@devmotion I'll let you do the corresponding release?

torfjelde · 2022-07-23T17:19:41Z

src/multivariate/product.jl

+        Base.depwarn(
+            "`Product(v)` is deprecated, please use `product_distribution(v)`",
+            :Product,
+        )


Maybe I'm just brainfarting, but it seems like this combined with the @deprecate below means that no matter which constructor I use for a vector of univariate distributions, I'm going to get a deprecation-warning?

Yes, at the moment one always get's a depwarn ... #1589

Yeah it's unfortunate and fixed by #1590. I thought someone would approve the PR and I would be able to make a bugfix release shortly after the issue was discovered but it seems nobody has approved it within almost two weeks. I'm going to merge it and tag a release now since the issue is quite annoying for downstream packages (and AFAICT even causes time outs in Turing).

Thanks @devmotion !

Wonderful stuff @devmotion !

Move src/multivariate/product.jl

34cd1ac

devmotion force-pushed the dw/product branch from 7c90922 to 5870272 Compare August 31, 2021 20:10

Generalize Product to ProductDistribution

2758c6a

devmotion force-pushed the dw/product branch from 5870272 to 2758c6a Compare August 31, 2021 20:25

Add implementations for more general product distributions

541e265

devmotion added 2 commits November 9, 2021 23:50

Merge branch 'master' into dw/product

5c7a82a

Unify and generalize rand!, logpdf and pdf

c951804

devmotion force-pushed the dw/product branch from ccf4f8e to c951804 Compare November 14, 2021 01:27

Merge branch 'master' into dw/product

f4d9184

devmotion added 4 commits November 14, 2021 23:32

Revert unrelated changes and fix tests

39ad15e

Propagate @inbounds

5417bb5

Remove unneeded implementation

0ce8ee3

Fix typos

b0a369d

devmotion added 8 commits November 15, 2021 00:31

Fix some dispatches

52bd470

More fixes

bf140cc

Support tuple of distributions and mix of discrete + continuous

343d266

Fix additional test errors

120c3f1

Fix method ambiguity

867a21e

Fix VonMisesFisherSampler

f86549b

Fix mixture sampler

6847f71

Simplify multinomial sampler

84ee30c

devmotion added 2 commits November 28, 2021 17:21

Merge branch 'master' into dw/product

b3c6a9c

Remove duplicate eachvariate/EachVariate

2e7341b

devmotion changed the title ~~WIP: Generalize Product and MatrixReshaped~~ WIP: Generalize Product Nov 28, 2021

devmotion and others added 5 commits November 28, 2021 21:45

Reintroduce Product

97d3a0a

Improve type inference

167f319

Add explanations of ValueSupport

b190369

Fix typo

e1fa4b5

Remove another breaking change

55e6c5b

devmotion changed the title ~~WIP: Generalize Product~~ Generalize Product Nov 29, 2021

This was referenced Dec 16, 2021

Replaces #1437 - additional documentation for filldist and arraydist TuringLang/Turing.jl#1751

Merged

Add Slab-and-Spike prior TuringLang/Turing.jl#847

Closed

mschauer mentioned this pull request Jan 12, 2022

Constructing a censored distribution #1468

Closed

mschauer reviewed Jan 18, 2022

View reviewed changes

yebai mentioned this pull request Feb 3, 2022

Fix maporbroadcast for ReverseDiff TuringLang/Bijectors.jl#142

Closed

Merge branch 'master' into dw/product

1a574fa

devmotion mentioned this pull request Jun 3, 2022

Allow Product to represent independent multivariate distributions #1562

Closed

mschauer approved these changes Jul 7, 2022

View reviewed changes

matbesancon merged commit c430f39 into master Jul 7, 2022

matbesancon deleted the dw/product branch July 7, 2022 11:10

torfjelde reviewed Jul 23, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generalize `Product` #1391

Generalize `Product` #1391

devmotion commented Aug 31, 2021 •

edited

Loading

oschulz commented Aug 31, 2021

devmotion commented Aug 31, 2021 •

edited

Loading

oschulz commented Aug 31, 2021

codecov-commenter commented Aug 31, 2021 •

edited

Loading

matbesancon commented Oct 5, 2021

oschulz commented Nov 7, 2021

devmotion commented Nov 7, 2021

mschauer commented Nov 14, 2021

oschulz commented Nov 14, 2021

devmotion commented Nov 27, 2021

cscherrer commented Nov 27, 2021

oschulz commented Nov 28, 2021

oschulz commented Nov 28, 2021

cscherrer commented Nov 28, 2021

devmotion commented Nov 30, 2021

mschauer Jan 18, 2022

oschulz commented Jun 3, 2022

devmotion commented Jul 7, 2022

oschulz commented Jul 7, 2022

matbesancon commented Jul 7, 2022

torfjelde Jul 23, 2022

oschulz Jul 23, 2022

devmotion Jul 23, 2022

oschulz Jul 23, 2022

torfjelde Jul 24, 2022

Generalize Product #1391

Generalize Product #1391

Conversation

devmotion commented Aug 31, 2021 • edited Loading

oschulz commented Aug 31, 2021

devmotion commented Aug 31, 2021 • edited Loading

oschulz commented Aug 31, 2021

codecov-commenter commented Aug 31, 2021 • edited Loading

Codecov Report

matbesancon commented Oct 5, 2021

oschulz commented Nov 7, 2021

devmotion commented Nov 7, 2021

mschauer commented Nov 14, 2021

oschulz commented Nov 14, 2021

devmotion commented Nov 27, 2021

cscherrer commented Nov 27, 2021

oschulz commented Nov 28, 2021

oschulz commented Nov 28, 2021

cscherrer commented Nov 28, 2021

devmotion commented Nov 30, 2021

mschauer Jan 18, 2022

Choose a reason for hiding this comment

oschulz commented Jun 3, 2022

devmotion commented Jul 7, 2022

oschulz commented Jul 7, 2022

matbesancon commented Jul 7, 2022

torfjelde Jul 23, 2022

Choose a reason for hiding this comment

oschulz Jul 23, 2022

Choose a reason for hiding this comment

devmotion Jul 23, 2022

Choose a reason for hiding this comment

oschulz Jul 23, 2022

Choose a reason for hiding this comment

torfjelde Jul 24, 2022

Choose a reason for hiding this comment

Generalize `Product` #1391

Generalize `Product` #1391

devmotion commented Aug 31, 2021 •

edited

Loading

devmotion commented Aug 31, 2021 •

edited

Loading

codecov-commenter commented Aug 31, 2021 •

edited

Loading