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

unconditionally get/put RNG state #825

Merged
merged 2 commits into from Feb 28, 2018
Merged

unconditionally get/put RNG state #825

merged 2 commits into from Feb 28, 2018

Conversation

@kevinushey
Copy link
Contributor

@kevinushey kevinushey commented Feb 27, 2018

Resolves #823. With this PR, we ensure that Rcpp functions called by other Rcpp functions synchronize the RNG state.

Although @wch's example showed this through the use of Rcpp::Function, in theory this could occur if an Rcpp function were to e.g. call source() or eval() or another API that happened to call another Rcpp function.

@eddelbuettel

This comment has been minimized.

Copy link
Member

@eddelbuettel eddelbuettel commented on src/api.cpp in 573e71d Feb 26, 2018

If we do this, we should probably also remove the RNGScopeCounter variable.

@kevinushey
Copy link
Contributor Author

@kevinushey kevinushey commented Feb 27, 2018

I wanted to leave the counter there just because it's part of the API of Rcpp (in particular, they're registered routines) and I want to avoid touching anything around that interface just out of an abundance of caution.

@eddelbuettel
Copy link
Member

@eddelbuettel eddelbuettel commented Feb 27, 2018

That seems ... weird. The entire block is https://github.com/RcppCore/Rcpp/blob/master/src/api.cpp#L66-L83 and the counter is not part of the interface. We could simply make it const. I just don't see why you want to increment/decrement it. AFAIK no other code touches it.

And FWIW the silversearcher agrees:

edd@rob:~/git/rcpp(master)$ ag  RNGScopeCounter
src/api.cpp
68:            unsigned long RNGScopeCounter = 0;
73:            if (RNGScopeCounter == 0) GetRNGstate();
74:            RNGScopeCounter++;
75:            return RNGScopeCounter;
80:            RNGScopeCounter--;
81:            if (RNGScopeCounter == 0) PutRNGstate();
82:            return RNGScopeCounter;
edd@rob:~/git/rcpp(master)$ 
@kevinushey
Copy link
Contributor Author

@kevinushey kevinushey commented Feb 28, 2018

Sorry; I meant that enterRNGScope() and exitRNGScope() are registered routines (just because we wanted to make sure that all packages using Rcpp were looking at the same counter, I presume).

I'll get rid of the counter.

@eddelbuettel
Copy link
Member

@eddelbuettel eddelbuettel commented Feb 28, 2018

And I presume you and @wch tested this and ensured that it addressed the corner case seen with shiny?

@eddelbuettel eddelbuettel merged commit fc8ebec into master Feb 28, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@eddelbuettel eddelbuettel deleted the rng-state branch Mar 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.