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

Expand the testing coverage using R as the reference #414

Merged
merged 33 commits into from
Feb 3, 2017
Merged

Conversation

lindahua
Copy link
Contributor

@lindahua lindahua commented Sep 6, 2015

Currently, we use Python's Scipy package as a reference to verify the correctness of our implementation of distributions.

However, Scipy only provides a limited coverage of what we currently have. This PR aims to expand the coverage of our testing by switching to using R as our reference. R, together with the packages in CRAN, can cover most (if not all) of the distributions.

@lindahua lindahua added this to the v0.9 milestone Sep 6, 2015
@johnmyleswhite
Copy link
Member

Good call

@ararslan
Copy link
Member

ararslan commented Feb 1, 2017

Thanks for all of your work here, @lindahua, this is great. Glad to see you around these parts again. 🙂

@lindahua
Copy link
Contributor Author

lindahua commented Feb 2, 2017

This PR is ready, and passed all tests in my machine.

Now almost all distributions have been covered by this R-based reference framework, including many distributions that were not available in scipy, e.g. Generalized Extreme Value, Beta Binomial, Noncentral Beta/Chi squared/F/T, Noncentral Hypergeometric, Normal Inverse Gaussian, and Von Mises, etc.

Also, with the new test cases, several bugs have been found and fixed.

A readme describing this testing framework was provided.

@lindahua lindahua changed the title [WIP] Expand the testing coverage using R as the reference Expand the testing coverage using R as the reference Feb 2, 2017
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.

Great PR. Thanks for that and good to see you back here. Just two minor comments though I haven't reviewed the R part.

end

kurtosis{T<:Real}(d::TriangularDist{T}) = T(-3)/5

entropy(d::TriangularDist) = 1//2 + log((d.b - d.a) / 2)
entropy(d::TriangularDist) = 0.5 + log((d.b - d.a) / 2)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't use Float64 literal here to avoid promoting e.g. Float32 input. Maybe one(d.b)/2.

@@ -82,12 +82,12 @@ end

function skewness(d::TriangularDist)
(a, b, c) = params(d)
sqrt2 * (a + b - 2c) * (2a - b - c) * (a - 2b + c) / (5 * _pretvar(a, b, c)^3//2)
sqrt2 * (a + b - 2c) * (2a - b - c) * (a - 2b + c) / (5 * _pretvar(a, b, c)^1.5)
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@lindahua
Copy link
Contributor Author

lindahua commented Feb 3, 2017

@andreasnoack Fix per your comments.

@andreasnoack andreasnoack merged commit 83c063d into master Feb 3, 2017
@andreasnoack andreasnoack deleted the dh/rtest branch February 3, 2017 20:43
@andreasnoack
Copy link
Member

The failures on master are because of JuliaLang/julia#20344

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