Add singular branch of the Wishart #1097
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1097 +/- ##
==========================================
+ Coverage 79.54% 79.70% +0.16%
==========================================
Files 113 113
Lines 5787 5834 +47
==========================================
+ Hits 4603 4650 +47
Misses 1184 1184
Continue to review full report at Codecov.
|
| # ----------------------------------------------------------------------------- | ||
|
|
||
| function Wishart(df::T, S::AbstractPDMat{T}) where T<:Real | ||
| function Wishart(df::T, S::AbstractPDMat{T}, warn::Bool = true) where T<:Real |
There was a problem hiding this comment.
a warning for expected functionality is not so nice
There was a problem hiding this comment.
Fair, and I'm happy to get rid of this.
I guess I had in mind that this functionality is more unexpected for most users, so I wanted to give them a heads up.
There was a problem hiding this comment.
Is it possible to remove the warning? I agree that it's probably not good to warn the users if Distributions supports this functionality, maybe it would be better to just mention it clearly in show(::Wishart)? Additionally, it is inconsistent with the behaviour of Normal(4.0, 0.0) and other special cases. An additional benefit of not printing warnings would be that the constructor would be compatible with Zygote, as noted in TuringLang/DistributionsAD.jl#84 (although probably that is not a good argument since Distributions does not try to support Zygote in the first place).
There was a problem hiding this comment.
maybe it would be better to just mention it clearly in show(::Wishart)?
Yes, great idea. Will do that soon. The warning was really intended more in the spirit of a deprecation: a temporary heads up about functionality people might not be expecting.
although probably that is not a good argument since Distributions does not try to support Zygote in the first place
Agreed, but I am in favor of thinking through reasonable changes that Distributions could make that would allow Turing to simply add methods to Distributions.Wishart and so on instead of reimplementing it. I'll maybe open an issue about that someplace.
Co-authored-by: Moritz Schauer <moritzschauer@web.de>
|
Thanks for the help @mschauer. I guess if there are no objections I'll merge this in 24 hours? |
4 participants
This PR adds the singular branch of the Wishart distribution. That is, for integer
df < p, the Wishart is a distribution over rank-dfpositive semidefinitep x pmatrices, defined in the usual way.In this case the Wishart does not have a Lebesgue density, but it does have a density with respect to an appropriate dominating measure on rank-
dfpositive semidefinite matrices, which is given in Theorem 6 here. The usual density in the full rank case is a special case of this "generalized" density.I am currently implementing this by extending the behavior of
Wishartinstead of defining a separate distribution. I prefer this because it's what Matlab does, and because I think that a hard distinction between singular/nonsingular Wishart is ultimately artificial and could cause headache for the user. For example, there are certain time series models where the degrees of freedom of a Wishart are time-varying, and may or may not be singular from period to period. A user implementing this might prefer that we supply one Wishart that covers both cases instead of supplying two and forcing them to manually switch between them.Let me know what you think!