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

`DiscreteNonParametric` and `Categorical` Construction Issue

Open btmit opened this issue 1 year ago • 4 comments

Construction of a Categorical distribution seems to make a copy of the p vector. I see this through profiling, @btime and the fact that I can't see changes in the original vector after I create the Categorical. There are three issues I see:

  1. Categorical docstring includes the following: "Note: The input vector p is directly used as a field of the constructed distribution, without being copied." which seems incorrect.
  2. Performance issues in critical sections of code where this allocation can really add up
  3. Bugs such as the following:
using Distributions
x = rand(3,5)
x = x ./ sum(x, dims=1)  # each column is a valid probability vector
c = Categorical.(eachcol(x))

julia> c = Categorical.(eachcol(x)) ERROR: MethodError: Cannot convert an object of type Vector{Float64} to an object of type SubArray{Float64, 1, Matrix{Float64}, Tuple{Base.Slice{Base.OneTo{Int64}}, Int64}, true}

I believe the underlying issue is that the DiscreteNonParametric inner constructor tries to sort and reorder everything, which creates a copy and then the constructor doesn't update the type.

btmit avatar Jan 30 '24 18:01 btmit

Just an anecdote, I'm hitting the performance implication of this issue. I'm running discrete event simulations that construct Categorical distributions from several statistical models 100s of billions of times. The ability to reuse p would help greatly here. Alternatively, supplying a tuple instead of a vector would work too.

JockLawrie avatar Jan 08 '25 23:01 JockLawrie

Did you compare it with https://github.com/JuliaStats/Distributions.jl/pull/1908?

devmotion avatar Jan 08 '25 23:01 devmotion

Yes PR 1908 looks like it fixes this issue, thanks. Is there anything blocking it being merged?

JockLawrie avatar Jan 09 '25 03:01 JockLawrie

It hasn't been approved yet.

devmotion avatar Jan 09 '25 07:01 devmotion