Differentiating mvnormal

[matbesancon]

[codecov-commenter]

Codecov Report

Base: 85.95% // Head: 86.02% // Increases project coverage by +0.06% :tada:

Coverage data is based on head (8ebf419) compared to base (a31ebc4). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1554      +/-   ##
==========================================
+ Coverage   85.95%   86.02%   +0.06%     
==========================================
  Files         129      129              
  Lines        8105     8144      +39     
==========================================
+ Hits         6967     7006      +39     
  Misses       1138     1138              

Impacted Files	Coverage Δ
src/multivariate/mvnormal.jl	80.93% <100.00%> (+3.41%)	:arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[matbesancon]

│   %34 = Base.getproperty(∂d_sq::Tangent{FullNormal}, :μ)::Any
│   %35 = (0.5 * %34)::Any
│   %36 = (%33 - %35)::Any
│   %37 = Base.getproperty(∂d_c0, :Σ)::Matrix{Float64}
│   %38 = Base.getproperty(∂d_sq::Tangent{FullNormal}, :Σ)::Any

One of the instability issues seems to be that the type of the members of Tangent{FullNormal} don't seem to be inferrable

[matbesancon]

Alright I could fix inference but it took some assumptions which might be a bit restrictive

[matbesancon]

@devmotion I think everything discussed has been adapted/validated

[matbesancon]

not computing the inverse at all would be a bit of a hassle here since it's reused several times in the whole function. I removed the materialization of the inverse as a matrix here to lighten the computations, we are using the inverse of the Cholesky decomposition directly

[matbesancon]

That change here: https://github.com/JuliaStats/Distributions.jl/pull/1554/commits/b225298aad7d57f96d560da7606987b3cd41e434 was somehow incorrect

[matbesancon]

Reverted the changes, the inverse covariance matrix is needed here, there is no way around it since the derivative is just the invcov scaled

[matbesancon]

Are the rules for sqmahal etc. needed? Would it maybe be sufficient to just make sure that PDMats works and is optimized for ChainRules?

I would say not since the operations done on top are non-trivial and could be costly

[matbesancon]

ping @devmotion for another round of review