Skip to content

Fix EmpiricalUnivariateDistribution #662

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

Closed
wants to merge 1 commit into from

Conversation

juliohm
Copy link
Contributor

@juliohm juliohm commented Sep 2, 2017

This PR should fix the quantile and rand of EmpiricalUnivariateDistribution. I have also changed the distribution to be discrete instead of continuous because it relies on the ecdf from StatsBase.jl, which doesn't perform interpolation.

Could you please review the changes and see if I got all the corner cases right? I am bisecting the CDF values in order to find the quantile, I think this is the best we can do?

@andreasnoack
Copy link
Member

andreasnoack commented Sep 2, 2017

Remember to check for open PRs, see #661

@juliohm
Copy link
Contributor Author

juliohm commented Sep 2, 2017

Oh, didn't see that, I told you in Discourse that I would submit a PR, that is why I didn't even check.

I like that you are using quantile(d.values, p) instead of reimplementing the bisection, I will close this one.

@juliohm juliohm closed this Sep 2, 2017
@andreasnoack
Copy link
Member

Oh, didn't see that, I told you in Discourse that I would submit a PR

No worries. I started on this yesterday right after I realized how broken the existing version was.

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.

2 participants