Open aerdely opened 1 year ago
That fallback seems dangerous. Maybe we should remove it and replace it with individual methods for distributions that have a single mode?
Yes. I was just about to make that point. The fallback inappropriately assumes a single mode. A more accurate approach would be to check the pdfs across the support of a distribution:
function modes(d::DiscreteUnivariateDistribution)
_support = minimum(d):maximum(d)
pdfs = pdf.(d, _support)
max_pdf,_ = findmax(pdfs)
indices = map(p -> p ≈ max_pdf, pdfs)
return _support[indices]
end
Brute force is inefficient, but it's better than efficiently arriving at an incorrect result. Perhaps someone could make some improvements to the above code, but that general approach would be better. Otherwise, I agree with @nalimilan, the fallback should be removed unless it always provides an accurate result.
minimum(d):maximum(d)
returns a range with a unit step, so it will give extremely inexact results and is unlikely to ever identify two modes even if they exist. Conversely, going over all possible float values will take forever. We should define specific methods for each distribution instead, and if we don't have a good solution for some distributions throwing an error is better.
My mistake. I updated the example above so that it is restricted to DiscreteUnivariateDistribution
. But I think the issue would still apply in cases where the range is large or unbounded.
The package does not calculate correctly the theoretical mode and/or modes under models like
Binomial
,Poisson
andNegativeBinomial
here some examples: