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

Make symbols in deprecation warnings more explicit #1428

Merged
merged 4 commits into from
Nov 18, 2021
Merged

Make symbols in deprecation warnings more explicit #1428

merged 4 commits into from
Nov 18, 2021

Conversation

rikhuijzer
Copy link
Contributor

The deprecation warning read:

┌ Warning: `MvNormal(d::Int, σ::Real)` is deprecated, use `MvNormal(Diagonal(Fill(σ ^ 2, d)))` instead.

However, Fill is not exported by Distributions and is likely to cause much searching for many readers.

This PR fixes it by using fill instead of Fill. Another solution would be to export Fill in src/Distributions.jl.

cc: @devmotion

@devmotion
Copy link
Member

No, I definitely think users should use Fill - otherwise the behaviour is different than before and their code will become allocating.

Also a no on the second point, we agreed to not re-export FillArrays (and other dependencies) but instead document them clearly: #1368

@devmotion
Copy link
Member

I wonder if the deprecation message becomes clearer if we use LinearAlgebra.Diagonal and FillArrays.Fill in the @deprecated definition. Ideally users should see

┌ Warning: `MvNormal(d::Int, σ::Real)` is deprecated, use `MvNormal(LinearAlgebra.Diagonal(FillArrays.Fill(σ ^ 2, d)))` instead.

which would match our (ideal) documentation (all external structs and functions are qualified).

@devmotion
Copy link
Member

It seems to work locally (don't pay attention to the stupid example):

julia> using LinearAlgebra

julia> g(x) = sum(abs2, x)
g (generic function with 1 method)

julia> Base.@deprecate f(x) g(LinearAlgebra.Diagonal(x))
f (generic function with 1 method)

julia> f(rand(3, 3))
┌ Warning: f(x) is deprecated, use g(LinearAlgebra.Diagonal(x)) instead.
│   caller = top-level scope at REPL[4]:1
└ @ Core REPL[4]:1
0.2616974132900723

@rikhuijzer
Copy link
Contributor Author

I wonder if the deprecation message becomes clearer if we use LinearAlgebra.Diagonal and FillArrays.Fill in the @deprecated definition. Ideally users should see

┌ Warning: `MvNormal(d::Int, σ::Real)` is deprecated, use `MvNormal(LinearAlgebra.Diagonal(FillArrays.Fill(σ ^ 2, d)))` instead.

which would match our (ideal) documentation (all external structs and functions are qualified).

I love it!

Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good! While you're at it, maybe update the deprecations in mvnormalcanon.jl as well?

@rikhuijzer rikhuijzer changed the title Use fill instead of Fill Make symbols in deprecation warnings more explicit Nov 18, 2021
Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! I'll merge when tests pass.

@devmotion
Copy link
Member

Test errors are unrelated (recent release of PDMats uncovered a bug in the implementation of Wishart).

@devmotion devmotion merged commit b2aa838 into JuliaStats:master Nov 18, 2021
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.

2 participants