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

R Changes RNG #312

Closed
jarioksa opened this issue Apr 22, 2019 · 6 comments
Closed

R Changes RNG #312

jarioksa opened this issue Apr 22, 2019 · 6 comments
Labels

Comments

@jarioksa
Copy link
Contributor

@jarioksa jarioksa commented Apr 22, 2019

On 22 Apr 2019, at 14:37, Prof Brian Ripley ripley@stats.ox.ac.uk wrote:

R 3.6.0 will be released on Friday and contains changes to the random sample() generation which affects comparison with reference output (mainly .Rout.save files) in these packages. Since CRAN expects clean output with current R (rather than past nor future versions), please update the package as soon as possible after Friday -- you may want to make the package depend on R (>= 3.6.0) as several packages currently in 3.6.0/Other have done. Before May 12 to safely retain the package on CRAN

@jarioksa
Copy link
Contributor Author

@jarioksa jarioksa commented May 7, 2019

We had used our own way of getting random index in src/nestedness.c from R dobule unif_rand(). R 3.6.0 release NEWS tell that C function R_unif_index() is now considered as public API, and we should switch to this function in our src/nestedness.c. I have done so in branch try-R_unif_index.

In the current version, I enclosed the new code in #ifdef ... #endif for R >= 3.6.0 so that our old IRAND macro will be used on older R. I also added a test function test_sample() without #ifdef, but to my surprise, it also worked in older R. It seems that R_unif_index() was defined already in R 3.4.0, but not made a part of official API, and so the compilation passes also in old R. However, it is R 3.6.0 that contains the fixed code.

How absolute should we be with requesting this change? I know that @gavinsimpson would like to make permute dependent on R >= 3.6.0, and because vegan depends on permute, that would in effect also make vegan dependent on R >= 3.6.0. The reason is that the results are more correct (more uniformly distributed) than in older R. @psolymos : what do you think with this change in src/nestedness.c?

Surely for correctness we should depend on R 3.6.0. The only problem is that R 3.6.0 is burning hot new, and not all people can upgrade (as their ICT staff upgrades when they will) or they don't want to upgrade. What ever we do, I think that we should make a concerted effort with permute.

jarioksa added a commit that referenced this issue May 8, 2019
R 3.4.0 is the first that exported R_unif_index in its C API,
although this was advertised as a part of public API only in
R 3.6.0. Although R_unif_index is available retroactively in
pre-3.6.0, the R_unif_index code was improved in R 3.6.0 and
naturally it is wise to upgrade (but that's none of vegan's
business). However, now code is made equal to the corresponding
R code and its base::sample() function. See issue #312.
@gavinsimpson
Copy link
Contributor

@gavinsimpson gavinsimpson commented May 8, 2019

We (Jari and I) had a rather length discussion on this over in the permute repo (gavinsimpson/permute#25). I got the impression Jari that you weren't keen on making vegan depend on R >= 3.6.0 and I respect that and I think we had come to some mutual understanding that I'd not force a dependency on R >= 3.6.0 but that I would change permute to emit a startup message upon loading on R < 3.6.0 to indicate that there was bias in the RNG prior to R v3.6.0 and that you might like to upgrade R but that the bias is likely small for modest numbers of observations.

I'm content to stick with that solution but, if vegan were made dependent on R >= 3.6.0 I would also change permute to likewise depend on that version of R.

Whatever we need to do, we need to decide soon as CRAN wanted these things fixed by May 12.

@psolymos
Copy link
Contributor

@psolymos psolymos commented May 8, 2019

@jarioksa and @gavinsimpson : vegan depending on R >= 3.6.x seems like a good idea and the RNG fixes will likely result in lots of other packages following course. So riding that wave might cause less headache for users who at some point will have to upgrade for this very reason.

Given that vegan runs on R 3.4.0 with the new C API, and that permute can be patched without declaring R >= 3.6.0 dependency explicitly, which gives time for R version laggards (myself included on some machines).

I am definitely in favour of relying on the fixed RNG and new API, the question is timing. Should we wait until 3.6.1? Looking at release dates of the 3.x.x series it will come out in June or July.

@jarioksa
Copy link
Contributor Author

@jarioksa jarioksa commented May 8, 2019

@psolymos Brian Ripley urged as to submit a new version of vegan by May 12 (which is mothers' day over here and pretty soon). I have now a version that explicitly depends on R 3.4.0 in try-R_unif_rand branch, but this is not yet merged to master or cran-2.5 branches. In this version the IRAND macro in nestedness.c uses R_unif_index function of R. I think this is improvement in all versions of R (also in 3.4.x, 3.5.x), and the 3.6.0 version should be still improved. Anyway, we need to submit something pretty soon, but we may quite well submit another version fairly soon, and then we may concern upping the dependence to R 3.6.0 simultaneously with permute (if that suits @gavinsimpson ).

@gavinsimpson
Copy link
Contributor

@gavinsimpson gavinsimpson commented May 8, 2019

Sounds good; let's wait until 3.6.1 is released before we make a final decision on whether to depend on R >= 3.6.0.

I'll soon push an update to permute to throw the startup message on R < 3.6.0 if I can get it to pass checks cleanly on all systems CRAN checks on.

jarioksa added a commit that referenced this issue May 9, 2019
See issue #312
jarioksa added a commit that referenced this issue May 9, 2019
@jarioksa
Copy link
Contributor Author

@jarioksa jarioksa commented May 13, 2019

vegan 2.5-5 was released on CRAN. See NEWS.

@jarioksa jarioksa closed this May 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.