Distributions.jl icon indicating copy to clipboard operation
Distributions.jl copied to clipboard

Incorrect uses of @inbounds cause miscalculation of statistics

Open yurivish opened this issue 4 years ago • 3 comments

There are many incorrect uses of @inbounds in this package, leading to incorrect results when fitting distributions and doing other calculations on arrays with offset axes.

I came across this issue previously with DiscreteUniform but did not realize how widespread the problem was until I ran a search for @inbounds across the code in this package.

Many uses of @inbounds in this package are in functions that accept arbitrary abstract vectors, matrices, or arrays, then disable bounds checks and index into the array with unsafe one-based indices.

One example is fitting a Normal distribution:

julia> using Distributions, OffsetArrays

julia> a = collect(-5:5);

julia> b = OffsetArray(a, -5:5);

julia> suffstats(Normal, a)
Distributions.NormalStats(0.0, 0.0, 110.0, 11.0)

julia> fit(Normal, a)
Normal{Float64}(μ=0.0, σ=3.1622776601683795)

julia> suffstats(Normal, b)
Distributions.NormalStats(7.39673056934e11, 6.7243005175818184e10, 1.5901446116594743e23, 11.0)

julia> fit(Normal, b)
Normal{Float64}(μ=6.7243005175818184e10, σ=1.2023252515852448e11)

https://github.com/JuliaStats/Distributions.jl/blob/863844c88e4153af13996f571fcc612d159de542/src/univariate/continuous/normal.jl#L254-L271

julia> versioninfo()
Julia Version 1.5.3
Commit 788b2c77c1 (2020-11-09 13:37 UTC)
Platform Info:
  OS: macOS (x86_64-apple-darwin18.7.0)
  CPU: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-9.0.1 (ORCJIT, skylake)

yurivish avatar Jan 23 '21 08:01 yurivish

Are there any uses of inbounds left after this PR, @itsdebartha?

ParadaCarleton avatar Sep 08 '23 00:09 ParadaCarleton

@ParadaCarleton Yes, there are still some uses of inbounds in categorical, discretenonparametric and poissonbinomial for the univariates. They are a bit tricky to tackle...

itsdebartha avatar Sep 16 '23 04:09 itsdebartha