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.

Sign up for GitHub

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

Couple documentation fixes #1847

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+10 −4
Open

Couple documentation fixes #1847

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
92085f1
Fix univariate `loglikelihood` doc
cgarling Apr 2, 2024
1b5a62e
Fix multivariate `pdf` and `logpdf` docs
cgarling Apr 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/src/multivariate.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ entropy(::MultivariateDistribution, ::Real)

```@docs
insupport(::MultivariateDistribution, ::AbstractArray)
pdf(::MultivariateDistribution, ::AbstractArray)
logpdf(::MultivariateDistribution, ::AbstractArray)
pdf(::Distribution{ArrayLikeVariate{N}}, x::AbstractArray{<:Real,M}) where {N,M}
logpdf(::Distribution{ArrayLikeVariate{N}}, x::AbstractArray{<:Real,M}) where {N,M}
Comment on lines +34 to +35
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure... This page is about MultivariateDistribution but the methods listed here are for generic distributions with array-like variates. I think it would be better to include these generic docstrings in a separate page and only list here pdf, logpdf etc. as part of the interface for multivariate distributions with a link to the generic docstring.

Copy link
Author

Choose a reason for hiding this comment

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

That's a valid concern, I was just following the docstring for loglikelihood for array-like variates which is currently listed on the multivariate page, so I thought it would be consistent.

Distributions.jl/src/common.jl

Lines 433 to 443 in f33af97

"""
loglikelihood(d::Distribution{ArrayLikeVariate{N}}, x) where {N}
The log-likelihood of distribution `d` with respect to all variate(s) contained in `x`.
Here, `x` can be any output of `rand(d, dims...)` and `rand!(d, x)`. For instance, `x` can
be
- an array of dimension `N` with `size(x) == size(d)`,
- an array of dimension `N + 1` with `size(x)[1:N] == size(d)`, or
- an array of arrays `xi` of dimension `N` with `size(xi) == size(d)`.
"""

If you think these should be taken to a separate page, then loglikelihood probably should as well

loglikelihood(::MultivariateDistribution, ::AbstractVector{<:Real})
```
**Note:** For multivariate distributions, the pdf value is usually very small or large, and therefore direct evaluation of the pdf may cause numerical problems. It is generally advisable to perform probability computation in log scale.
Expand Down
2 changes: 1 addition & 1 deletion docs/src/univariate.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ pdfsquaredL2norm
insupport(::UnivariateDistribution, x::Any)
pdf(::UnivariateDistribution, ::Real)
logpdf(::UnivariateDistribution, ::Real)
loglikelihood(::UnivariateDistribution, ::AbstractArray)
loglikelihood(::UnivariateDistribution, ::Real)
Copy link
Member

Choose a reason for hiding this comment

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

You almost never want to call loglikelihood with ::Real but rather with ::AbstractArray, so documenting the former instead of the latter seems wrong.

Copy link
Member

Choose a reason for hiding this comment

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

Where is that method?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the review. It is not obvious to me why the ::AbstractArray signature would be preferred over broadcasting through the ::Real signature for UnivariateDistribution. My naive go-to would be the ::Real method as it is available for all UnivariateDistribution and the ::AbstractArray signature is not. You could make the docstring more useful by adding a sentence explaining why you may or may not want to use it.

Copy link
Member

Choose a reason for hiding this comment

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

The canonical method for single variates is just logpdf. The real benefit of loglikelihood is that it sums the logpdf values for multiple variates, possibly using exploiting constant terms etc.

Copy link
Member

Choose a reason for hiding this comment

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

loglikelihood is defined here:

Distributions.jl/src/common.jl

Lines 444 to 465 in f33af97

Base.@propagate_inbounds @inline function loglikelihood(
d::Distribution{ArrayLikeVariate{N}}, x::AbstractArray{<:Real,M},
) where {N,M}
if M == N
return logpdf(d, x)
else
@boundscheck begin
M > N ||
throw(DimensionMismatch(
"number of dimensions of the variates ($M) must be greater than or equal to the dimension of the distribution ($N)"
))
ntuple(i -> size(x, i), Val(N)) == size(d) ||
throw(DimensionMismatch("inconsistent array dimensions"))
end
return @inbounds sum(Base.Fix1(logpdf, d), eachvariate(x, ArrayLikeVariate{N}))
end
end
Base.@propagate_inbounds function loglikelihood(
d::Distribution{ArrayLikeVariate{N}}, x::AbstractArray{<:AbstractArray{<:Real,N}},
) where {N}
return sum(Base.Fix1(logpdf, d), x)
end

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I was confused about "ArrayLikeVariate{N}", does this include univariate distributions?

Copy link
Author

Choose a reason for hiding this comment

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

Oh, I see, it calls down to

Distributions.jl/src/common.jl

Lines 434 to 465 in f33af97

loglikelihood(d::Distribution{ArrayLikeVariate{N}}, x) where {N}
The log-likelihood of distribution `d` with respect to all variate(s) contained in `x`.
Here, `x` can be any output of `rand(d, dims...)` and `rand!(d, x)`. For instance, `x` can
be
- an array of dimension `N` with `size(x) == size(d)`,
- an array of dimension `N + 1` with `size(x)[1:N] == size(d)`, or
- an array of arrays `xi` of dimension `N` with `size(xi) == size(d)`.
"""
Base.@propagate_inbounds @inline function loglikelihood(
d::Distribution{ArrayLikeVariate{N}}, x::AbstractArray{<:Real,M},
) where {N,M}
if M == N
return logpdf(d, x)
else
@boundscheck begin
M > N ||
throw(DimensionMismatch(
"number of dimensions of the variates ($M) must be greater than or equal to the dimension of the distribution ($N)"
))
ntuple(i -> size(x, i), Val(N)) == size(d) ||
throw(DimensionMismatch("inconsistent array dimensions"))
end
return @inbounds sum(Base.Fix1(logpdf, d), eachvariate(x, ArrayLikeVariate{N}))
end
end
Base.@propagate_inbounds function loglikelihood(
d::Distribution{ArrayLikeVariate{N}}, x::AbstractArray{<:AbstractArray{<:Real,N}},
) where {N}
return sum(Base.Fix1(logpdf, d), x)
end

because UnivariateDistribution <: Distribution{ArrayLikeVariate{0}}
For some reason I thought the univariate case was defined differently...

It seems like the real issue might be how we go about linking these general methods into the separate markdown pages, as the current @docs strings no longer work with the new methods. I don't really feel comfortable doing that myself, I don't understand the internals all that well (as shown above...)

cdf(::UnivariateDistribution, ::Real)
logcdf(::UnivariateDistribution, ::Real)
logdiffcdf(::UnivariateDistribution, ::Real, ::Real)
Expand Down
8 changes: 7 additions & 1 deletion src/univariates.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,13 @@ logpdf(d::UnivariateDistribution, x::Real)
# extract value from array of zero dimension
logpdf(d::UnivariateDistribution, x::AbstractArray{<:Real,0}) = logpdf(d, first(x))

# loglikelihood for `Real`
"""
loglikelihood(d::UnivariateDistribution, x::Real)

Evaluate the logarithm of the likelihood at `x`.

See also: [`logpdf`](@ref).
"""
Comment on lines +329 to +335
Copy link
Member

Choose a reason for hiding this comment

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

Same here, this is just a fallback for the case of a single variate but not the typical use case of loglikelihood.

Base.@propagate_inbounds loglikelihood(d::UnivariateDistribution, x::Real) = logpdf(d, x)

"""
Expand Down
Loading