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

Fix the constructor of `DiscreteNonParametric`

Open devmotion opened this issue 1 year ago • 2 comments

The constructor of DiscreteNonParametric always sorts the support, but this can change the type of the inputs - which means that the constructor either has to lie about the type of the constructed object or (as done currently) has to try to convert them back to the type of the input arguments. The latter fails e.g. for SubArrays.

IMO an inner constructor should generally not sort inputs but only check whether they are sorted. Therefore this PR removes sorting from the inner constructor and moves it to the outer constructor, and a check for whether the support vector is sorted is added to the inner constructor.

Additionally, to make construction more efficient the PR

  • skips sorting for support vectors that are statically known to be sorted (currently only AbstractUnitRange but this could be refactored into a static_issorted(::AbstractVector) function if more types show up)
  • adds a utility function issorted_allunique that checks efficiently whether a vector is sorted and contains only unique elements (ie its elements are strictly monotonically increasing) (for AbstractUnitRange issorted_allunique just returns true, so is a no-op).

Fixes #1084. Fixes #1832. Fixes #1878 (but no test is added since it would require an additional dependency on RecursiveArrayTools).

devmotion avatar Oct 02 '24 13:10 devmotion

Codecov Report

Attention: Patch coverage is 96.15385% with 1 line in your changes missing coverage. Please review.

Project coverage is 86.29%. Comparing base (da3a230) to head (3ec7b8f).

Files with missing lines Patch % Lines
src/utils.jl 92.30% 1 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1908      +/-   ##
==========================================
+ Coverage   86.27%   86.29%   +0.02%     
==========================================
  Files         146      146              
  Lines        8777     8800      +23     
==========================================
+ Hits         7572     7594      +22     
- Misses       1205     1206       +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov-commenter avatar Oct 02 '24 14:10 codecov-commenter

Not sure about the original motivation, apparently already the initial commit in https://github.com/JuliaStats/Distributions.jl/pull/634 required the support to be sorted and I couldn't find any discussion about it in that PR. I always assumed it was done to speed up a few relevant functions such as cdf etc. and a few more obscure ones as == or isapprox.

devmotion avatar Nov 22 '24 10:11 devmotion