Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the constructor of DiscreteNonParametric #1908

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

devmotion
Copy link
Member

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).

@codecov-commenter
Copy link

codecov-commenter commented Oct 2, 2024

Codecov Report

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

Project coverage is 86.10%. Comparing base (a1010e4) to head (b7283e7).

Files with missing lines Patch % Lines
src/utils.jl 92.30% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1908      +/-   ##
==========================================
+ Coverage   86.08%   86.10%   +0.02%     
==========================================
  Files         144      144              
  Lines        8696     8718      +22     
==========================================
+ Hits         7486     7507      +21     
- Misses       1210     1211       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@andreasnoack andreasnoack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it that the support needs to be sorted? I can see the docstring defines the support like that but do we know the motivation?

@devmotion
Copy link
Member Author

Not sure about the original motivation, apparently already the initial commit in #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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants