JuliaStats / Distributions.jl

A Julia package for probability distributions and associated functions.
Other
1.1k stars 414 forks source link

Incorrect uses of @inbounds cause miscalculation of statistics #1265

Open yurivish opened 3 years ago

yurivish commented 3 years ago

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)
ParadaCarleton commented 1 year ago

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

itsdebartha commented 1 year ago

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