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

Add PieceweiseLinearDist #1367

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

Conversation

davidanthoff
Copy link
Contributor

The idea is that one manually specifies points on the CDF, and anything in-between these points is then linearly interpolated. At the moment I require the first and last point to have a y value of 0 and 1 respectively, I think anything else doesn't really make sense?

I'll add tests and docs later, but wanted to get some feedback first whether a) the maintainers think this belongs in this package and b) get some additional eyes on the implementation and check whether I made some stupid mistake somewhere.

@codecov-commenter
Copy link

codecov-commenter commented Jul 16, 2021

Codecov Report

Merging #1367 (674ff9d) into master (02f1da3) will decrease coverage by 0.50%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1367      +/-   ##
==========================================
- Coverage   82.79%   82.28%   -0.51%     
==========================================
  Files         116      117       +1     
  Lines        6667     6708      +41     
==========================================
  Hits         5520     5520              
- Misses       1147     1188      +41     
Impacted Files Coverage Δ
src/univariate/continuous/piecewiselinear.jl 0.00% <0.00%> (ø)
src/univariates.jl 74.46% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02f1da3...674ff9d. Read the comment docs.

@andreasnoack
Copy link
Member

andreasnoack commented Jul 19, 2021

Thanks for opening the PR. A couple of immediate high-level comment: I believe this distributions is the same as the one proposed in #518 and #877. I think it's more common to use the shape of the density to inspire the name (e.g. semicircle and triangular) so "piecewise uniform" as proposed in #518 seems more appropriate to me than "piecewise linear".

@ParadaCarleton
Copy link
Contributor

ParadaCarleton commented Aug 9, 2021

I think this definitely looks like a good idea, but I think we should parametrize slightly differently. Uniform is currently specified with upper and lower endpoints. To align with that, PiecewiseUniform could be naturally implemented by having users provide the endpoints of each interval, with the distribution being uniform between these endpoints. I think that's more consistent, since the specification is in terms of the PDF rather than the CDF, like we have for the triangular and uniform distributions. It also generalizes to higher dimensions, where users can just provide a set of vertices that partition their space.

@ParadaCarleton
Copy link
Contributor

(Worth noting that because of this convention that random variables should be described in terms of their PDF, PiecewiseLinear as a name suggests to me a PDF that is piecewise linear, i.e. it should look like a bunch of trapezoids.)

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

Successfully merging this pull request may close these issues.

4 participants