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

Deprecate use of vectors + scalars of standard deviations in constructors of multivariate normal distributions #1362

Merged
merged 2 commits into from
Jul 15, 2021

Conversation

devmotion
Copy link
Member

There are multiple and regular complaints about the current constructors of MvNormal (and hence also of MvNormalCanon and MvLogNormal) being suprising and unintuitive (see, e.g., #1203, #1333, #584, #826, #657).

This PR implements the suggestion in #1203 (comment) and #1203 (comment), and deprecates the constructors of MvNormal that accept a scalar standard deviation or a vector of standard deviations. Instead one should use

MvNormal(mu::AbstractVector{<:Real}, Sigma::AbstractMatrix{<:Real})

and

MvNormal(Sigma::AbstractMatrix{<:Real})

for distributions with zero mean. If the deprecations are removed, this will allow us to define

MvNormal(mu::AbstractVector{<:Real}) = MvNormal(mu, I)

which would fix the inconsistencies with the constructor of Normal. More concretely, the deprecations are:

Base.@deprecate MvNormal::AbstractVector{<:Real}, σ::AbstractVector{<:Real}) MvNormal(μ, Diagonal(map(abs2, σ)))
Base.@deprecate MvNormal::AbstractVector{<:Real}, σ::Real) MvNormal(μ, σ^2 * I)
Base.@deprecate MvNormal::AbstractVector{<:Real}) MvNormal(Diagonal(map(abs2, σ)))
Base.@deprecate MvNormal(d::Int, σ::Real) MvNormal(Diagonal(Fill^2, d)))

In the same way, the constructors of MvNormalCanon and MvLogNormal are updated and deprecated.

@codecov-commenter
Copy link

codecov-commenter commented Jul 9, 2021

Codecov Report

Merging #1362 (355cc8e) into master (8c7f400) will increase coverage by 0.03%.
The diff coverage is 95.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1362      +/-   ##
==========================================
+ Coverage   82.76%   82.79%   +0.03%     
==========================================
  Files         116      116              
  Lines        6671     6667       -4     
==========================================
- Hits         5521     5520       -1     
+ Misses       1150     1147       -3     
Impacted Files Coverage Δ
src/multivariate/mvnormalcanon.jl 82.45% <92.85%> (ø)
src/multivariate/mvlognormal.jl 98.63% <100.00%> (ø)
src/multivariate/mvnormal.jl 74.07% <100.00%> (+1.01%) ⬆️

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 8c7f400...355cc8e. Read the comment docs.

@torfjelde
Copy link
Contributor

torfjelde commented Jul 17, 2021

I don't know why this causes issues, but it seems like this PR is causing Turing.jl tests to grind to a halt TuringLang/Turing.jl#1660 (comment). It might be an issue on the Turing.jl side (maybe DistributionsAD.jl?) that interacts badly with this change, but I figured I'd mention it here asap in case it also affect other packages.

@torfjelde
Copy link
Contributor

I figured out what the "bug" is. With --depwarn=yes:

julia> using Distributions, BenchmarkTools

julia> @benchmark $(MvNormal)($(zeros(1)), $(ones(1)))
┌ Warning: `MvNormal(μ::AbstractVector{<:Real}, σ::AbstractVector{<:Real})` is deprecated, use `MvNormal(μ, Diagonal(map(abs2, σ)))` instead.
│   caller = ip:0x0
└ @ Core :-1
BenchmarkTools.Trial: 
  memory estimate:  27.44 KiB
  allocs estimate:  363
  --------------
  minimum time:     64.617 ms (0.00% GC)
  median time:      67.703 ms (0.00% GC)
  mean time:        68.065 ms (0.00% GC)
  maximum time:     85.660 ms (0.00% GC)
  --------------
  samples:          74
  evals/sample:     1

without --depwarn=yes:

julia> @benchmark $(MvNormal)($(zeros(1)), $(ones(1)))
BenchmarkTools.Trial: 
  memory estimate:  192 bytes
  allocs estimate:  2
  --------------
  minimum time:     81.207 ns (0.00% GC)
  median time:      84.698 ns (0.00% GC)
  mean time:        96.957 ns (10.22% GC)
  maximum time:     3.617 μs (96.73% GC)
  --------------
  samples:          10000
  evals/sample:     962

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.

5 participants