From 40f40bcdc4ea7d476a2c5fa664957df658e3df73 Mon Sep 17 00:00:00 2001 From: David Widmann <devmotion@users.noreply.github.com> Date: Fri, 8 Dec 2023 12:04:41 +0100 Subject: [PATCH] Fix method ambiguities & unbound parameters + add Aqua tests (#1804) * Fix method ambiguities & unbound parameters + add Aqua tests * Fix compat entries for Julia 1.3 * Update aqua.jl --- Project.toml | 14 +++- README.md | 1 + src/censored.jl | 1 + src/common.jl | 107 ++++++++++++------------- src/deprecates.jl | 6 +- src/mixtures/mixturemodel.jl | 16 ++-- src/multivariate/mvtdist.jl | 2 +- src/product.jl | 14 ++-- src/univariate/discrete/categorical.jl | 2 +- src/univariates.jl | 6 +- test/aqua.jl | 21 +++++ test/runtests.jl | 6 +- 12 files changed, 112 insertions(+), 84 deletions(-) create mode 100644 test/aqua.jl diff --git a/Project.toml b/Project.toml index 68b96caa8f..df9f7004cc 100644 --- a/Project.toml +++ b/Project.toml @@ -30,15 +30,26 @@ DistributionsDensityInterfaceExt = "DensityInterface" DistributionsTestExt = "Test" [compat] +Aqua = "0.8" +Calculus = "0.5" ChainRulesCore = "1" +ChainRulesTestUtils = "1" DensityInterface = "0.4" +Distributed = "<0.0.1, 1" FillArrays = "0.9, 0.10, 0.11, 0.12, 0.13, 1" +FiniteDifferences = "0.12" +ForwardDiff = "0.10" +JSON = "0.21" LinearAlgebra = "<0.0.1, 1" +OffsetArrays = "1" PDMats = "0.10, 0.11" Printf = "<0.0.1, 1" QuadGK = "2" Random = "<0.0.1, 1" +SparseArrays = "<0.0.1, 1" SpecialFunctions = "1.2, 2" +StableRNGs = "1" +StaticArrays = "1" Statistics = "1" StatsAPI = "1.6" StatsBase = "0.32, 0.33, 0.34" @@ -47,6 +58,7 @@ Test = "<0.0.1, 1" julia = "1.3" [extras] +Aqua = "4c88cf16-eb10-579e-8560-4a9242c79595" Calculus = "49dc2e85-a5d0-5ad3-a950-438e2897f1b9" ChainRulesCore = "d360d2e6-b24c-11e9-a2a3-2a2ae2dbcce4" ChainRulesTestUtils = "cdddcdb0-9152-4a09-a978-84456f9df70a" @@ -62,4 +74,4 @@ StaticArrays = "90137ffa-7385-5640-81b9-e52037218182" Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40" [targets] -test = ["StableRNGs", "Calculus", "ChainRulesCore", "ChainRulesTestUtils", "DensityInterface", "Distributed", "FiniteDifferences", "ForwardDiff", "JSON", "SparseArrays", "StaticArrays", "Test", "OffsetArrays"] +test = ["Aqua", "StableRNGs", "Calculus", "ChainRulesCore", "ChainRulesTestUtils", "DensityInterface", "Distributed", "FiniteDifferences", "ForwardDiff", "JSON", "SparseArrays", "StaticArrays", "Test", "OffsetArrays"] diff --git a/README.md b/README.md index 80a39ecb72..0071cf3617 100644 --- a/README.md +++ b/README.md @@ -4,6 +4,7 @@ Distributions.jl [](https://github.com/JuliaStats/Distributions.jl/actions) [](https://zenodo.org/record/2647458) [](https://coveralls.io/r/JuliaStats/Distributions.jl?branch=master) +[](https://github.com/JuliaTesting/Aqua.jl) [](https://JuliaStats.github.io/Distributions.jl/latest/) [](https://JuliaStats.github.io/Distributions.jl/stable/) diff --git a/src/censored.jl b/src/censored.jl index 2d5e1d67fa..5b8cfac5ee 100644 --- a/src/censored.jl +++ b/src/censored.jl @@ -431,6 +431,7 @@ _in_open_interval(x::Real, l::Real, ::Nothing) = x > l _clamp(x, l, u) = clamp(x, l, u) _clamp(x, ::Nothing, u) = min(x, u) _clamp(x, l, ::Nothing) = max(x, l) +_clamp(x, ::Nothing, u::Nothing) = x _to_truncated(d::Censored) = truncated(d.uncensored, d.lower, d.upper) diff --git a/src/common.jl b/src/common.jl index 703e126930..31a218e197 100644 --- a/src/common.jl +++ b/src/common.jl @@ -212,13 +212,25 @@ usually it is sufficient to implement `logpdf`. See also: [`logpdf`](@ref). """ @inline function pdf( - d::Distribution{ArrayLikeVariate{N}}, x::AbstractArray{<:Real,N} -) where {N} - @boundscheck begin - size(x) == size(d) || - throw(DimensionMismatch("inconsistent array dimensions")) + d::Distribution{ArrayLikeVariate{N}}, x::AbstractArray{<:Real,M} +) where {N,M} + if M == N + @boundscheck begin + size(x) == size(d) || + throw(DimensionMismatch("inconsistent array dimensions")) + end + return _pdf(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 map(Base.Fix1(pdf, d), eachvariate(x, variate_form(typeof(d)))) end - return _pdf(d, x) end function _pdf(d::Distribution{ArrayLikeVariate{N}}, x::AbstractArray{<:Real,N}) where {N} @@ -241,13 +253,25 @@ size of `x`. See also: [`pdf`](@ref). """ @inline function logpdf( - d::Distribution{ArrayLikeVariate{N}}, x::AbstractArray{<:Real,N} -) where {N} - @boundscheck begin - size(x) == size(d) || - throw(DimensionMismatch("inconsistent array dimensions")) + d::Distribution{ArrayLikeVariate{N}}, x::AbstractArray{<:Real,M} +) where {N,M} + if M == N + @boundscheck begin + size(x) == size(d) || + throw(DimensionMismatch("inconsistent array dimensions")) + end + 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 map(Base.Fix1(logpdf, d), eachvariate(x, variate_form(typeof(d)))) end - return _logpdf(d, x) end # `_logpdf` should be implemented and has no default definition @@ -272,20 +296,6 @@ Base.@propagate_inbounds function pdf( return map(Base.Fix1(pdf, d), x) end -@inline function pdf( - d::Distribution{ArrayLikeVariate{N}}, x::AbstractArray{<:Real,M}, -) where {N,M} - @boundscheck begin - M > N || - throw(DimensionMismatch( - "number of dimensions of `x` ($M) must be greater than number of dimensions of `d` ($N)" - )) - ntuple(i -> size(x, i), Val(N)) == size(d) || - throw(DimensionMismatch("inconsistent array dimensions")) - end - return @inbounds map(Base.Fix1(pdf, d), eachvariate(x, variate_form(typeof(d)))) -end - """ logpdf(d::Distribution{ArrayLikeVariate{N}}, x) where {N} @@ -305,20 +315,6 @@ Base.@propagate_inbounds function logpdf( return map(Base.Fix1(logpdf, d), x) end -@inline function logpdf( - d::Distribution{ArrayLikeVariate{N}}, x::AbstractArray{<:Real,M}, -) where {N,M} - @boundscheck begin - M > N || - throw(DimensionMismatch( - "number of dimensions of `x` ($M) must be greater than number of dimensions of `d` ($N)" - )) - ntuple(i -> size(x, i), Val(N)) == size(d) || - throw(DimensionMismatch("inconsistent array dimensions")) - end - return @inbounds map(Base.Fix1(logpdf, d), eachvariate(x, variate_form(typeof(d)))) -end - """ pdf!(out, d::Distribution{ArrayLikeVariate{N}}, x) where {N} @@ -365,7 +361,7 @@ end @boundscheck begin M > N || throw(DimensionMismatch( - "number of dimensions of `x` ($M) must be greater than number of dimensions of `d` ($N)" + "number of dimensions of the variates ($M) must be greater than the dimension of the distribution ($N)" )) ntuple(i -> size(x, i), Val(N)) == size(d) || throw(DimensionMismatch("inconsistent array dimensions")) @@ -414,7 +410,7 @@ See also: [`pdf!`](@ref). @boundscheck begin M > N || throw(DimensionMismatch( - "number of dimensions of `x` ($M) must be greater than number of dimensions of `d` ($N)" + "number of dimensions of the variates ($M) must be greater than the dimension of the distribution ($N)" )) ntuple(i -> size(x, i), Val(N)) == size(d) || throw(DimensionMismatch("inconsistent array dimensions")) @@ -445,23 +441,22 @@ be - 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 function loglikelihood( - d::Distribution{ArrayLikeVariate{N}}, x::AbstractArray{<:Real,N}, -) where {N} - return logpdf(d, x) -end -@inline function loglikelihood( +Base.@propagate_inbounds @inline function loglikelihood( d::Distribution{ArrayLikeVariate{N}}, x::AbstractArray{<:Real,M}, ) where {N,M} - @boundscheck begin - M > N || - throw(DimensionMismatch( - "number of dimensions of `x` ($M) must be greater than number of dimensions of `d` ($N)" - )) - ntuple(i -> size(x, i), Val(N)) == size(d) || - throw(DimensionMismatch("inconsistent array dimensions")) + 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 - return @inbounds sum(Base.Fix1(logpdf, d), eachvariate(x, ArrayLikeVariate{N})) end Base.@propagate_inbounds function loglikelihood( d::Distribution{ArrayLikeVariate{N}}, x::AbstractArray{<:AbstractArray{<:Real,N}}, diff --git a/src/deprecates.jl b/src/deprecates.jl index e779d6dbb0..cacaa12b59 100644 --- a/src/deprecates.jl +++ b/src/deprecates.jl @@ -34,9 +34,9 @@ for fun in [:pdf, :logpdf, fun! = Symbol(fun, '!') @eval begin - @deprecate ($_fun!)(r::AbstractArray, d::UnivariateDistribution, X::AbstractArray) r .= ($fun).(d, X) false - @deprecate ($fun!)(r::AbstractArray, d::UnivariateDistribution, X::AbstractArray) r .= ($fun).(d, X) false - @deprecate ($fun)(d::UnivariateDistribution, X::AbstractArray) ($fun).(d, X) + @deprecate ($_fun!)(r::AbstractArray{<:Real}, d::UnivariateDistribution, X::AbstractArray{<:Real}) r .= ($fun).(d, X) false + @deprecate ($fun!)(r::AbstractArray{<:Real}, d::UnivariateDistribution, X::AbstractArray{<:Real}) r .= ($fun).(d, X) false + @deprecate ($fun)(d::UnivariateDistribution, X::AbstractArray{<:Real}) ($fun).(d, X) end end diff --git a/src/mixtures/mixturemodel.jl b/src/mixtures/mixturemodel.jl index 7f5f6782e3..fc05012d0d 100644 --- a/src/mixtures/mixturemodel.jl +++ b/src/mixtures/mixturemodel.jl @@ -362,14 +362,14 @@ end pdf(d::UnivariateMixture, x::Real) = _mixpdf1(d, x) logpdf(d::UnivariateMixture, x::Real) = _mixlogpdf1(d, x) -_pdf!(r::AbstractArray, d::UnivariateMixture{Discrete}, x::UnitRange) = _mixpdf!(r, d, x) -_pdf!(r::AbstractArray, d::UnivariateMixture, x::AbstractArray) = _mixpdf!(r, d, x) -_logpdf!(r::AbstractArray, d::UnivariateMixture, x::AbstractArray) = _mixlogpdf!(r, d, x) - -_pdf(d::MultivariateMixture, x::AbstractVector) = _mixpdf1(d, x) -_logpdf(d::MultivariateMixture, x::AbstractVector) = _mixlogpdf1(d, x) -_pdf!(r::AbstractArray, d::MultivariateMixture, x::AbstractMatrix) = _mixpdf!(r, d, x) -_logpdf!(r::AbstractArray, d::MultivariateMixture, x::AbstractMatrix) = _mixlogpdf!(r, d, x) +_pdf!(r::AbstractArray{<:Real}, d::UnivariateMixture{Discrete}, x::UnitRange) = _mixpdf!(r, d, x) +_pdf!(r::AbstractArray{<:Real}, d::UnivariateMixture, x::AbstractArray{<:Real}) = _mixpdf!(r, d, x) +_logpdf!(r::AbstractArray{<:Real}, d::UnivariateMixture, x::AbstractArray{<:Real}) = _mixlogpdf!(r, d, x) + +_pdf(d::MultivariateMixture, x::AbstractVector{<:Real}) = _mixpdf1(d, x) +_logpdf(d::MultivariateMixture, x::AbstractVector{<:Real}) = _mixlogpdf1(d, x) +_pdf!(r::AbstractArray{<:Real}, d::MultivariateMixture, x::AbstractMatrix{<:Real}) = _mixpdf!(r, d, x) +_logpdf!(r::AbstractArray{<:Real}, d::MultivariateMixture, x::AbstractMatrix{<:Real}) = _mixlogpdf!(r, d, x) ## component-wise pdf and logpdf diff --git a/src/multivariate/mvtdist.jl b/src/multivariate/mvtdist.jl index dbd13ce60f..9076c364b5 100644 --- a/src/multivariate/mvtdist.jl +++ b/src/multivariate/mvtdist.jl @@ -139,7 +139,7 @@ function _logpdf(d::AbstractMvTDist, x::AbstractVector{T}) where T<:Real v - shdfhdim * log1p(sqmahal(d, x) / d.df) end -function _logpdf!(r::AbstractArray, d::AbstractMvTDist, x::AbstractMatrix) +function _logpdf!(r::AbstractArray{<:Real}, d::AbstractMvTDist, x::AbstractMatrix{<:Real}) sqmahal!(r, d, x) shdfhdim, v = mvtdist_consts(d) for i = 1:size(x, 2) diff --git a/src/product.jl b/src/product.jl index 7a4904ae7a..d6f7aaa6bf 100644 --- a/src/product.jl +++ b/src/product.jl @@ -12,7 +12,9 @@ struct ProductDistribution{N,M,D,S<:ValueSupport,T} <: Distribution{ArrayLikeVar size::Dims{N} function ProductDistribution{N,M,D}(dists::D) where {N,M,D} - isempty(dists) && error("product distribution must consist of at least one distribution") + if isempty(dists) + throw(ArgumentError("a product distribution must consist of at least one distribution")) + end return new{N,M,D,_product_valuesupport(dists),_product_eltype(dists)}( dists, _product_size(dists), @@ -20,11 +22,11 @@ struct ProductDistribution{N,M,D,S<:ValueSupport,T} <: Distribution{ArrayLikeVar end end -function ProductDistribution(dists::AbstractArray{<:Distribution{ArrayLikeVariate{M}},N}) where {M,N} +function ProductDistribution(dists::AbstractArray{<:Distribution{<:ArrayLikeVariate{M}},N}) where {M,N} return ProductDistribution{M + N,M,typeof(dists)}(dists) end -function ProductDistribution(dists::NTuple{N,Distribution{ArrayLikeVariate{M}}}) where {M,N} +function ProductDistribution(dists::Tuple{Distribution{<:ArrayLikeVariate{M}},Vararg{Distribution{<:ArrayLikeVariate{M}}}}) where {M} return ProductDistribution{M + 1,M,typeof(dists)}(dists) end @@ -54,10 +56,10 @@ function _product_size(dists::AbstractArray{<:Distribution{<:ArrayLikeVariate{M} size_dists = size(dists) return ntuple(i -> i <= M ? size_d[i] : size_dists[i-M], Val(M + N)) end -function _product_size(dists::NTuple{N,Distribution{<:ArrayLikeVariate{M}}}) where {M,N} +function _product_size(dists::Tuple{Distribution{<:ArrayLikeVariate{M}},Vararg{Distribution{<:ArrayLikeVariate{M}}, N}}) where {M,N} size_d = size(first(dists)) all(size(d) == size_d for d in dists) || error("all distributions must be of the same size") - return ntuple(i -> i <= M ? size_d[i] : N, Val(M + 1)) + return ntuple(i -> i <= M ? size_d[i] : N + 1, Val(M + 1)) end ## aliases @@ -167,7 +169,7 @@ function _rand!( end # `_logpdf` for arrays of distributions -# we have to fix a method ambiguity +# we have to fix some method ambiguities _logpdf(d::ProductDistribution{N}, x::AbstractArray{<:Real,N}) where {N} = __logpdf(d, x) _logpdf(d::ProductDistribution{2}, x::AbstractMatrix{<:Real}) = __logpdf(d, x) function __logpdf( diff --git a/src/univariate/discrete/categorical.jl b/src/univariate/discrete/categorical.jl index 9572acb17e..1ac338f6f0 100644 --- a/src/univariate/discrete/categorical.jl +++ b/src/univariate/discrete/categorical.jl @@ -89,7 +89,7 @@ function pdf(d::Categorical, x::Real) return insupport(d, x) ? ps[round(Int, x)] : zero(eltype(ps)) end -function _pdf!(r::AbstractArray, d::Categorical{T}, rgn::UnitRange) where {T<:Real} +function _pdf!(r::AbstractArray{<:Real}, d::Categorical{T}, rgn::UnitRange) where {T<:Real} vfirst = round(Int, first(rgn)) vlast = round(Int, last(rgn)) vl = max(vfirst, 1) diff --git a/src/univariates.jl b/src/univariates.jl index 0e2ae05e32..ca83d727e5 100644 --- a/src/univariates.jl +++ b/src/univariates.jl @@ -311,7 +311,7 @@ See also: [`logpdf`](@ref). pdf(d::UnivariateDistribution, x::Real) = exp(logpdf(d, x)) # extract value from array of zero dimension -_pdf(d::UnivariateDistribution, x::AbstractArray{<:Real,0}) = pdf(d, first(x)) +pdf(d::UnivariateDistribution, x::AbstractArray{<:Real,0}) = pdf(d, first(x)) """ logpdf(d::UnivariateDistribution, x::Real) @@ -323,7 +323,7 @@ See also: [`pdf`](@ref). logpdf(d::UnivariateDistribution, x::Real) # extract value from array of zero dimension -_logpdf(d::UnivariateDistribution, x::AbstractArray{<:Real,0}) = logpdf(d, first(x)) +logpdf(d::UnivariateDistribution, x::AbstractArray{<:Real,0}) = logpdf(d, first(x)) # loglikelihood for `Real` Base.@propagate_inbounds loglikelihood(d::UnivariateDistribution, x::Real) = logpdf(d, x) @@ -452,7 +452,7 @@ function _pdf_fill_outside!(r::AbstractArray, d::DiscreteUnivariateDistribution, return vl, vr, vfirst, vlast end -function _pdf!(r::AbstractArray, d::DiscreteUnivariateDistribution, X::UnitRange) +function _pdf!(r::AbstractArray{<:Real}, d::DiscreteUnivariateDistribution, X::UnitRange) vl,vr, vfirst, vlast = _pdf_fill_outside!(r, d, X) # fill central part: with non-zero pdf diff --git a/test/aqua.jl b/test/aqua.jl new file mode 100644 index 0000000000..4dd8c07064 --- /dev/null +++ b/test/aqua.jl @@ -0,0 +1,21 @@ +using Distributions +using Test + +import Aqua + +@testset "Aqua" begin + # Test ambiguities separately without Base and Core + # Ref: https://github.com/JuliaTesting/Aqua.jl/issues/77 + Aqua.test_all( + Distributions; + ambiguities = false, + # On older Julia versions, installed dependencies are quite old + # Thus unbound type parameters show up that are fixed in newer versions + unbound_args = VERSION >= v"1.6", + ) + # Tests are not reliable on older Julia versions and + # show ambiguities in loaded packages + if VERSION >= v"1.6" + Aqua.test_ambiguities(Distributions) + end +end diff --git a/test/runtests.jl b/test/runtests.jl index ce3f16b799..583132c536 100644 --- a/test/runtests.jl +++ b/test/runtests.jl @@ -11,6 +11,7 @@ import JSON import ForwardDiff const tests = [ + "aqua", "univariate/continuous/loguniform", "univariate/continuous/arcsine", "univariate/discrete/dirac", @@ -174,8 +175,3 @@ include("testutils.jl") include("$t.jl") end end - -# print method ambiguities -println("Potentially stale exports: ") -display(Test.detect_ambiguities(Distributions)) -println()