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

Added random number generator unit tests (closes #28) #514

Merged
merged 3 commits into from Jul 24, 2016

Conversation

@coatless
Copy link
Contributor

commented Jul 22, 2016

Adds unit test for random generation for the R:: namespace where Rmath
is kept.

Added section breaks in C++ code for each distribution

Adds unit test for random generation for the R:: namespace where Rmath
is kept.

Added section breaks in C++ code for each distribution
@eddelbuettel

This comment has been minimized.

Copy link
Member

commented Jul 22, 2016

That's nice. Setting the seed 'on both sides' makes sure we didn't spill the milk.

@eddelbuettel

This comment has been minimized.

Copy link
Member

commented Jul 22, 2016

When I merge #513 this one will conflict as both alter Changelog.

@coatless

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2016

I'll resync with main branch and then reload this commit.

@coatless

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2016

Weird, works fine under the local check. I'll look into the failure.

I have a funny feeling it is due to the RNG seed generation difference.

https://travis-ci.org/RcppCore/Rcpp/builds/146754299#L4243-L4292

@eddelbuettel

This comment has been minimized.

Copy link
Member

commented Jul 22, 2016

But Travis 'just' running Ubuntu Linux too.

I think we can mod this part of .travis.yml:

after_failure:
  - ./travis-tool.sh dump_logs

and look into what the shell script for dump_logs and add to that.

…seene-Twister" and "Inversion" (the defaults).

Switched from = to <- in file.
@eddelbuettel

This comment has been minimized.

Copy link
Member

commented Jul 22, 2016

@coatless and I took this off-line and found that the concatenation of two subvectors, ie c(a1, a2) appears to results in c(a2, a1) making the comparison fail.

# Conflicts:
#	ChangeLog
@coatless

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2016

@eddelbuettel : Modified the test bit to avoid having to rev() the vector.

Cpp now generates under a for instead of ::create()

R now generates under the vectorized r*(n,...) functions.

Though, this exercise did reveal something interesting: The concatenation order differs between R & Cpp. I'm still trying to repo this on my end. Might be a bigger bug.

@eddelbuettel

This comment has been minimized.

Copy link
Member

commented Jul 22, 2016

Not sure it is a bug or a just a, well, hesitating, err, feature. The way you had lined up your expression, these were actually not yet evaluated. So we are racing against both concatenation and whether evaluation happens a certain way. Could be a tricky corner to play in.

@eddelbuettel

This comment has been minimized.

Copy link
Member

commented Jul 24, 2016

There is one thing that this now lacks (but which we can add): vectorised, ie sugar, accessors. Ie when you do

NumericVector o(5);

for(int i = 0; i < o.size(); i++)
        o[i] = R::rnorm(a, b);

you are testing the (scalar) accessor from Rmath via the R:: namespace. We also need

RcppNumericVector o = Rcpp::rnorm(5, a, b);

but that can be added.

@eddelbuettel eddelbuettel merged commit 4cee7ca into RcppCore:master Jul 24, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@eddelbuettel

This comment has been minimized.

Copy link
Member

commented Jul 24, 2016

I started to add some more in this new branch via this first commit.

@coatless coatless deleted the coatless:unit-test-rdist branch Jul 24, 2016
@coatless

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2016

@eddelbuettel I have no write access on that branch =(

@eddelbuettel

This comment has been minimized.

Copy link
Member

commented Jul 24, 2016

It's ok -- I'm already done.

@coatless

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2016

@eddelbuettel

Need to also do sugar testing for p/d/q * functions as well.

@eddelbuettel

This comment has been minimized.

Copy link
Member

commented Jul 24, 2016

Maybe, maybe not. The the very highest priority but we should keep it in mind. It is really just looping over scalar version. The 'r' variants are equally close but do have the RNG draw business so are potentially more easily breakable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.