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

Failures from reverse dependency checks #80

Closed
rstub opened this issue Apr 10, 2024 · 40 comments
Closed

Failures from reverse dependency checks #80

rstub opened this issue Apr 10, 2024 · 40 comments
Assignees

Comments

@rstub
Copy link
Member

rstub commented Apr 10, 2024

Reverse dependency checks via GHA are still not working, but running them locally gives me two failures:

GridOnClusters

Run revdepcheck::revdep_details(, "GridOnClusters") for more info

Newly broken

  • checking tests ...
      Running ‘testthat.R’
     ERROR
    Running the tests in ‘tests/testthat.R’ failed.
    Last 13 lines of output:
      Mean relative difference: 0.192
      ── Failure ('test-GridOnClusters.R:200:3'): Testing discretize.jointly ("kmeans+silhouette") ──
      `dim13` not equivalent to as.table(...).
      Mean relative difference: 0.182
      ── Failure ('test-GridOnClusters.R:208:3'): Testing discretize.jointly ("kmeans+silhouette") ──
      `dim23` not equivalent to as.table(...).
      Mean relative difference: 0.072
      ── Failure ('test-GridOnClusters.R:214:3'): Testing discretize.jointly ("kmeans+silhouette") ──
      round(discr$csimilarity, digits = 3) not equivalent to 0.915.
      1/1 mismatches
      [1] 0.849 - 0.915 == -0.066
      
      [ FAIL 30 | WARN 0 | SKIP 0 | PASS 10 ]
      Error: Test failures
      Execution halted
    

@joemsong: Could this be related to the change in default RNG? If I find an explanation: Do you have a way to accept a PR?

rnndescent

Run revdepcheck::revdep_details(, "rnndescent") for more info

Newly broken

  • checking whether package ‘rnndescent’ can be installed ... ERROR
    Installation failed.
    See ‘/home/ralf/devel/dqrng/revdep/checks/rnndescent/new/rnndescent.Rcheck/00install.out’ for details.
    

Newly fixed

  • checking installed package size ... NOTE
      installed size is 21.7Mb
      sub-directories of 1Mb or more:
        libs  20.6Mb
    

Installation

Devel

* installing *source* package ‘rnndescent’ ...
** package ‘rnndescent’ successfully unpacked and MD5 sums checked
** using staged installation
** libs
using C++ compiler: ‘g++ (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0’
using C++17
g++ -std=gnu++17 -I"/opt/R/4.3.0/lib/R/include" -DNDEBUG -I../inst/include/ -I'/home/ralf/devel/dqrng/revdep/library/dqrng/new/BH/include' -I'/home/ralf/devel/dqrng/revdep/library/dqrng/new/dqrng/include' -I'/home/ralf/devel/dqrng/revdep/library/dqrng/new/Rcpp/include' -I'/home/ralf/devel/dqrng/revdep/library/dqrng/new/sitmo/include' -I/usr/local/include   -DSTRICT_R_HEADERS -DRCPP_NO_MODULES -fpic  -g -O2  -c RcppExports.cpp -o RcppExports.o
g++ -std=gnu++17 -I"/opt/R/4.3.0/lib/R/include" -DNDEBUG -I../inst/include/ -I'/home/ralf/devel/dqrng/revdep/library/dqrng/new/BH/include' -I'/home/ralf/devel/dqrng/revdep/library/dqrng/new/dqrng/include' -I'/home/ralf/devel/dqrng/revdep/library/dqrng/new/Rcpp/include' -I'/home/ralf/devel/dqrng/revdep/library/dqrng/new/sitmo/include' -I/usr/local/include   -DSTRICT_R_HEADERS -DRCPP_NO_MODULES -fpic  -g -O2  -c rnn_bruteforce.cpp -o rnn_bruteforce.o
g++ -std=gnu++17 -I"/opt/R/4.3.0/lib/R/include" -DNDEBUG -I../inst/include/ -I'/home/ralf/devel/dqrng/revdep/library/dqrng/new/BH/include' -I'/home/ralf/devel/dqrng/revdep/library/dqrng/new/dqrng/include' -I'/home/ralf/devel/dqrng/revdep/library/dqrng/new/Rcpp/include' -I'/home/ralf/devel/dqrng/revdep/library/dqrng/new/sitmo/include' -I/usr/local/include   -DSTRICT_R_HEADERS -DRCPP_NO_MODULES -fpic  -g -O2  -c rnn_hub.cpp -o rnn_hub.o
g++ -std=gnu++17 -I"/opt/R/4.3.0/lib/R/include" -DNDEBUG -I../inst/include/ -I'/home/ralf/devel/dqrng/revdep/library/dqrng/new/BH/include' -I'/home/ralf/devel/dqrng/revdep/library/dqrng/new/dqrng/include' -I'/home/ralf/devel/dqrng/revdep/library/dqrng/new/Rcpp/include' -I'/home/ralf/devel/dqrng/revdep/library/dqrng/new/sitmo/include' -I/usr/local/include   -DSTRICT_R_HEADERS -DRCPP_NO_MODULES -fpic  -g -O2  -c rnn_indextograph.cpp -o rnn_indextograph.o
...
In file included from rnn_nnd.cpp:24:
../inst/include/rnndescent/random.h: In function ‘dqrng::rng64_t rnndescent::create_dqrng()’:
../inst/include/rnndescent/random.h:49:62: error: could not convert ‘std::make_shared(_Args&& ...) [with _Tp = dqrng::random_64bit_wrapper<pcg_detail::engine<long unsigned int, __int128 unsigned, pcg_detail::xsl_rr_mixin<long unsigned int, __int128 unsigned>, false, pcg_detail::specific_stream<__int128 unsigned>, pcg_detail::default_multiplier<__int128 unsigned> > >; _Args = {}]()’ from ‘std::shared_ptr<dqrng::random_64bit_wrapper<pcg_detail::engine<long unsigned int, __int128 unsigned, pcg_detail::xsl_rr_mixin<long unsigned int, __int128 unsigned>, false, pcg_detail::specific_stream<__int128 unsigned>, pcg_detail::default_multiplier<__int128 unsigned> > > >’ to ‘dqrng::rng64_t’ {aka ‘Rcpp::XPtr<dqrng::random_64bit_generator>’}
   49 |   return std::make_shared<dqrng::random_64bit_wrapper<pcg64>>();
      |          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
      |                                                              |
      |                                                              std::shared_ptr<dqrng::random_64bit_wrapper<pcg_detail::engine<long unsigned int, __int128 unsigned, pcg_detail::xsl_rr_mixin<long unsigned int, __int128 unsigned>, false, pcg_detail::specific_stream<__int128 unsigned>, pcg_detail::default_multiplier<__int128 unsigned> > > >
make: *** [/opt/R/4.3.0/lib/R/etc/Makeconf:200: rnn_nnd.o] Error 1
ERROR: compilation failed for package ‘rnndescent’
* removing ‘/home/ralf/devel/dqrng/revdep/checks/rnndescent/new/rnndescent.Rcheck/rnndescent’

@jlmelville This is clearly my redefinition of dqrng::rng64_t. It might make sense to use dqrng::generator<pcg64>() instead. I can provide a PR for that. BTW, given that I have factored out the sampling code into dqrng_sample.h: Do you still need your own copy dqsample.h?

@jlmelville
Copy link

@rstub thanks for the heads-up, I'll take a look at this as soon as I can

@rstub
Copy link
Member Author

rstub commented Apr 12, 2024

@jlmelville Great! The following patch works for me both with the CRAN version and the version from #79:

diff --git a/inst/include/rnndescent/random.h b/inst/include/rnndescent/random.h
index 9619b37..09b0b3c 100644
--- a/inst/include/rnndescent/random.h
+++ b/inst/include/rnndescent/random.h
@@ -46,7 +46,13 @@ inline auto r_seed() -> uint64_t {
 }
 
 inline auto create_dqrng() -> dqrng::rng64_t {
-  return std::make_shared<dqrng::random_64bit_wrapper<pcg64>>();
+  auto seed1 = r_seed();
+  auto seed2 = r_seed();
+  return dqrng::generator<pcg64>(seed1, seed2);
+}
+
+inline auto create_dqrng(uint64_t seed, uint64_t seed2) -> dqrng::rng64_t {
+  return dqrng::generator<pcg64>(seed, seed2);
 }
 
 inline auto combine_seeds(uint32_t msw, uint32_t lsw) -> uint64_t {
@@ -65,8 +71,7 @@ struct TauRand : public tdoann::RandomGenerator {
   std::unique_ptr<tdoann::tau_prng> prng{nullptr};
 
   TauRand(uint64_t seed, uint64_t seed2) {
-    dqrng::rng64_t rng = create_dqrng();
-    rng->seed(seed, seed2);
+    dqrng::rng64_t rng = create_dqrng(seed, seed2);
 
     // Stitch together 3 64-bit ints from 6 32-bit ones
     std::vector<uint32_t> tau_seeds32;
@@ -114,15 +119,9 @@ private:
 public:
 
   // Not thread safe
-  DQIntSampler() : rng(create_dqrng()) {
-    auto seed1 = r_seed();
-    auto seed2 = r_seed();
-    rng->seed(seed1, seed2);
-  }
+  DQIntSampler() : rng(create_dqrng()) {}
 
-  DQIntSampler(uint64_t seed, uint64_t seed2) : rng(create_dqrng()) {
-    rng->seed(seed, seed2);
-  }
+  DQIntSampler(uint64_t seed, uint64_t seed2) : rng(create_dqrng(seed, seed2)) {}
 
   // Generates a random integer in range [0, n)
   Int rand_int(Int n) override {

Feel free to use it. I can also provide a PR for it.

@rstub
Copy link
Member Author

rstub commented Apr 12, 2024

More packages seem to be affected. It looks like those are affected by the change in default RNG. Most likely it would help to add dqRNGkind("xoroshiro128+") before dqset.seed(...) in the test code:

FunChisq

@joemsong

* checking tests ... ERROR
  Running ‘testthat.R’
Running the tests in ‘tests/testthat.R’ failed.
Last 13 lines of output:
  round(xtoy$statistic, digits = 2) not equivalent to 53.48.
  1/1 mismatches
  [1] 55 - 53.5 == 1.5
  ── Failure ('test_AdpFunChisq.R:65:3'): Testing the adapted functional chi-squared test ──
  signif(xtoy$p.value, digits = 2) not equivalent to 0.81.
  1/1 mismatches
  [1] 0.87 - 0.81 == 0.06
  ── Failure ('test_AdpFunChisq.R:66:3'): Testing the adapted functional chi-squared test ──
  round(xtoy$statistic, digits = 2) not equivalent to 3.
  1/1 mismatches
  [1] 2.51 - 3 == -0.49
  
  [ FAIL 5 | WARN 0 | SKIP 0 | PASS 242 ]
  Error: Test failures
  Execution halted

fwildclusterboot

@s3alfisc

* checking tests ... ERROR
  Running ‘testthat.R’
Running the tests in ‘tests/testthat.R’ failed.
Last 13 lines of output:
    'test-r-vs-julia.R:4:3', 'test-seed.R:3:3', 'test-tstat_equivalence.R:335:3',
    'test-tstat_equivalence.R:561:3', 'test-tstat_equivalence.R:840:3',
    'test-uncategorized.R:3:3', 'test_sunab.R:3:3', 'test_tidy.R:3:3',
    'test_tidy.R:64:3'
  
  ══ Failed tests ════════════════════════════════════════════════════════════════
  ── Failure ('test_samplers.R:36:3'): test sampling ─────────────────────────────
  confint(boot1) (`actual`) not equal to confint(boot2) (`expected`).
  
    `actual`: -0.015 -0.000
  `expected`: -0.016 -0.000
  
  [ FAIL 1 | WARN 1 | SKIP 19 | PASS 180 ]
  Error: Test failures
  Execution halted

greeks

@ahudde

* checking tests ... ERROR
  Running ‘testthat.R’
Running the tests in ‘tests/testthat.R’ failed.
Last 13 lines of output:
  > library(greeks)
  > 
  > test_check("greeks")
  [1] "custom payoff"
  [1] "custom payoff"
  [1] "custom payoff"
  [ FAIL 1 | WARN 8 | SKIP 0 | PASS 20 ]
  
  ══ Failed tests ════════════════════════════════════════════════════════════════
  ── Failure ('test-Malliavin_European_Greeks.R:59:3'): Malliavin_European_Greeks is correct ──
  unknown failure
  
  [ FAIL 1 | WARN 8 | SKIP 0 | PASS 20 ]
  Error: Test failures
  Execution halted

@jlmelville
Copy link

@rstub is the current state of main the correct code for testing? When I use that your patch it does fix the compilation error, but I am now getting linker errors later:

   g++ -shared -static-libgcc -o rnndescent.dll tmp.def RcppExports.o rnn_bruteforce.o rnn_hub.o rnn_indextograph.o rnn_merge.o rnn_nnd.o rnn_prepare.o rnn_randnbrs.o rnn_rptree.o rnn_search.o rnn_util.o -LC:/rtools43/x86_64-w64-mingw32.static.posix/lib/x64 -LC:/rtools43/x86_64-w64-mingw32.static.posix/lib -LC:/PROGRA~1/R/R-43~1.3/bin/x64 -lR
   C:\rtools43\x86_64-w64-mingw32.static.posix\bin/ld.exe: rnn_prepare.o: in function `dqrng::random_64bit_accessor::random_64bit_accessor()':
   E:/dev/R/win-library/4.0/Rcpp/include/Rcpp/utils/tinyformat/tinyformat.h:664: multiple definition of `dqrng::random_64bit_accessor::random_64bit_accessor()'; rnn_nnd.o:E:/dev/R/win-library/4.0/Rcpp/include/Rcpp/utils/tinyformat/tinyformat.h:674: first defined here
   C:\rtools43\x86_64-w64-mingw32.static.posix\bin/ld.exe: rnn_prepare.o: in function `dqrng::random_64bit_accessor::random_64bit_accessor()':
   E:/dev/R/win-library/4.0/Rcpp/include/Rcpp/utils/tinyformat/tinyformat.h:664: multiple definition of `dqrng::random_64bit_accessor::random_64bit_accessor()'; rnn_nnd.o:E:/dev/R/win-library/4.0/Rcpp/include/Rcpp/utils/tinyformat/tinyformat.h:674: first defined here
   C:\rtools43\x86_64-w64-mingw32.static.posix\bin/ld.exe: rnn_randnbrs.o: in function `dqrng::random_64bit_accessor::random_64bit_accessor()':
   E:/dev/R/win-library/4.0/Rcpp/include/Rcpp/utils/tinyformat/tinyformat.h:674: multiple definition of `dqrng::random_64bit_accessor::random_64bit_accessor()'; rnn_nnd.o:E:/dev/R/win-library/4.0/Rcpp/include/Rcpp/utils/tinyformat/tinyformat.h:674: first defined here
   C:\rtools43\x86_64-w64-mingw32.static.posix\bin/ld.exe: rnn_randnbrs.o: in function `dqrng::random_64bit_accessor::random_64bit_accessor()':
   E:/dev/R/win-library/4.0/Rcpp/include/Rcpp/utils/tinyformat/tinyformat.h:674: multiple definition of `dqrng::random_64bit_accessor::random_64bit_accessor()'; rnn_nnd.o:E:/dev/R/win-library/4.0/Rcpp/include/Rcpp/utils/tinyformat/tinyformat.h:674: first defined here
   C:\rtools43\x86_64-w64-mingw32.static.posix\bin/ld.exe: rnn_rptree.o: in function `dqrng::random_64bit_accessor::random_64bit_accessor()':
   E:/dev/R/win-library/4.0/dqrng/include/dqrng.h:25: multiple definition of `dqrng::random_64bit_accessor::random_64bit_accessor()'; rnn_nnd.o:E:/dev/R/win-library/4.0/Rcpp/include/Rcpp/utils/tinyformat/tinyformat.h:674: first defined here
   C:\rtools43\x86_64-w64-mingw32.static.posix\bin/ld.exe: rnn_rptree.o: in function `dqrng::random_64bit_accessor::random_64bit_accessor()':
   E:/dev/R/win-library/4.0/dqrng/include/dqrng.h:25: multiple definition of `dqrng::random_64bit_accessor::random_64bit_accessor()'; rnn_nnd.o:E:/dev/R/win-library/4.0/Rcpp/include/Rcpp/utils/tinyformat/tinyformat.h:674: first defined here
   C:\rtools43\x86_64-w64-mingw32.static.posix\bin/ld.exe: rnn_search.o: in function `dqrng::random_64bit_accessor::random_64bit_accessor()':
   E:/dev/R/win-library/4.0/Rcpp/include/Rcpp/utils/tinyformat/tinyformat.h:664: multiple definition of `dqrng::random_64bit_accessor::random_64bit_accessor()'; rnn_nnd.o:E:/dev/R/win-library/4.0/Rcpp/include/Rcpp/utils/tinyformat/tinyformat.h:674: first defined here
   C:\rtools43\x86_64-w64-mingw32.static.posix\bin/ld.exe: rnn_search.o: in function `dqrng::random_64bit_accessor::random_64bit_accessor()':
   E:/dev/R/win-library/4.0/Rcpp/include/Rcpp/utils/tinyformat/tinyformat.h:664: multiple definition of `dqrng::random_64bit_accessor::random_64bit_accessor()'; rnn_nnd.o:E:/dev/R/win-library/4.0/Rcpp/include/Rcpp/utils/tinyformat/tinyformat.h:674: first defined here
   collect2.exe: error: ld returned 1 exit status
   no DLL was created
   ERROR: compilation failed for package 'rnndescent'

Also: yes I would be happy to see if I can stop using dqsample.h once I can build again.

@rstub
Copy link
Member Author

rstub commented Apr 13, 2024

@jlmelville Please use the state from #79 for testing. Current main is missing an important inline.

Concering dqsample.h: It is best to wait until I have done a CRAN release since dqrng_sample.h will change in that release. At the moment, it expects dqrng::rng64_t as argument and will change to dqrng::random_64bit_generator&. So it is important to have a version on CRAN that works both the CRAN of dqrng and the version from #79.

@jlmelville
Copy link

@jlmelville Please use the state from #79 for testing. Current main is missing an important inline.

Got it. It's working now on #79 and the CRAN release of dqrng. In terms of coordinating, do you want me to prep a new release of rnndescent to CRAN? Let me know when it would be ok to submit that.

Thank you for the patch. Do you object strongly to being listed as a contributor in rnndescent?

Concering dqsample.h: It is best to wait until I have done a CRAN release since dqrng_sample.h will change in that release. At the moment, it expects dqrng::rng64_t as argument and will change to dqrng::random_64bit_generator&.

Understood.

@rstub
Copy link
Member Author

rstub commented Apr 13, 2024

@jlmelville You could upload anytime, but it probably makes sense to wait until I have finalized #79. Feel free to add me as contributor if you see this as important enough.

@rstub
Copy link
Member Author

rstub commented Apr 15, 2024

@s3alfisc: I was able to reproduce the issue with the CRAN version 0.13 of fwildclusterboot, while both the current development version as well as version 0.14.3 from r-universe does not show this error. Do you have any plans for a CRAN release?
BTW, a minimal fix for v0.13 would be to insert dqrng::dgRNGkind("xoroshiro128+") in test_samplers.R.

@rstub
Copy link
Member Author

rstub commented Apr 15, 2024

@joemsong: I was able to reproduce the issue with the CRAN versions of both GridOnClusters and FunChisq. In both cases, it is enough to insert dqrng::dgRNGkind("xoroshiro128+") in the relevant test file. Are you interested in me providing a patch?

@rstub
Copy link
Member Author

rstub commented Apr 15, 2024

@ahudde: I was able to reproduce the issue with the CRAN version of greeks. It is enough to insert dqrng::dgRNGkind("xoroshiro128+") in the relevant test file. However, in the current development version I see a different failure which I cannot fix this way.

@s3alfisc
Copy link

Do you have any plans for a CRAN release?

Hi @rstub , I have no immediate plans for a CRAN release - would you like me to submit a hotfix to 0.13 by adding dqrng::dgRNGkind("xoroshiro128+")? Then I would tackle this tomorrow after work. Sorry for taking a few days to respond, I was traveling over the weekend.

@ahudde
Copy link

ahudde commented Apr 16, 2024

Dear @rstub,
Sorry for the inconvenience. And thank you very much for your great packages, which really helps improving performance of my package. I hope I can find the time to look into it the coming weekend.

@daqana daqana deleted a comment from ahudde Apr 17, 2024
@rstub
Copy link
Member Author

rstub commented Apr 17, 2024

Thanks @ahudde and @s3alfisc for looking into this! A CRAN upload of your packages in the coming weeks with an appropriate fix would be great.

@jlmelville
Copy link

@jlmelville You could upload anytime, but it probably makes sense to wait until I have finalized #79. Feel free to add me as contributor if you see this as important enough.

Just to confirm @rstub, as #79 is complete, would it now be helpful to submit a new version of rnndescent?

@rstub
Copy link
Member Author

rstub commented Apr 18, 2024

@jlmelville Yes, please go ahead.

@jlmelville
Copy link

@jlmelville Yes, please go ahead.

A new version of rnndescent (0.1.5) is now on CRAN. Hopefully it passes all the usual checks. If you are still doing reverse dependency checks it might be worth trying on the new version of rnndescent, just in case.

@rstub
Copy link
Member Author

rstub commented Apr 19, 2024

🎉 Latest revdep check no longer shows a failure with rnndescent. Interestingly, fwildclusterboot was fine as well, even though this is still the original version.

@rstub
Copy link
Member Author

rstub commented Apr 19, 2024

@s3alfisc: It is odd, that fwildclusterboot does not show any problems in the last revdep check. And looking at the previous results it was actually the CRAN version of dqrng that produced the error:

Package: fwildclusterboot
Check: tests
Old result: ERROR
New result: OK 

Looking at the test file in question at https://github.com/s3alfisc/fwildclusterboot/blob/master/tests/testthat/test_samplers.R it looks like you are not setting any seeds. Might this be the reason? Setting a fixed RNG would still make sense, though.

ahudde pushed a commit to ahudde/greeks that referenced this issue Apr 20, 2024
ahudde pushed a commit to ahudde/greeks that referenced this issue Apr 20, 2024
@ahudde
Copy link

ahudde commented Apr 24, 2024

Thanks @ahudde and @s3alfisc for looking into this! A CRAN upload of your packages in the coming weeks with an appropriate fix would be great.

Dear @rstub, a new greeks-Version where the tests work with the new dqrng-Version is now CRAN.

@rstub
Copy link
Member Author

rstub commented Apr 29, 2024

@jlmelville This is strange. I am currently seeing reverse dependency failures from rnndescent again. This time not from the installation but from the tests in test_rptree.R. Do you know what you are doing differently in that file compared with other tests?
As for things that have changed: The version from #79 already changed the way the two argument ctor for PCG64 is used. I changed that again in #82, but I would have expected that code that works with CRAN version and the version from #79 to also work with the version from #82, i.e. results might change slightly but the overall result should be the same. Any idea whay is going on?

@rstub rstub self-assigned this Apr 29, 2024
@jlmelville
Copy link

@rstub by building off the current main (I see #82 has been merged so I think this the right thing to test?), I can reproduce the test failures.

The tree tests are very sensitive to the results of the random number generator. So if for a given seed the random numbers generated by dqrng change, so will those results. If that's what's happened with #82, I will need to update those tests to match the new output. So that wouldn't be an unexpected failure.

But if the stream of random numbers wasn't supposed to change then I will need to do some debugging to generate the stream of random numbers before and after.

@rstub
Copy link
Member Author

rstub commented May 6, 2024

@jlmelville This is very odd. The results from the RNG for your use-case have changed multiple times, since I changed how the two argument construction dqrng::generator<pcg64>(seed, stream) works. AFAIK this is the only way how you are making use of RNGs from {dqrng}.

  1. The CRAN version 0.3.2 uses the two argument ctor pcg64(seed, stream) for this. You test cases are working with this version.

  2. In Add clone(stream) method for the RNGs #79 (merged in 6819b9a as version 0.3.2.4) I changed that to use pcg64(seed) plus pcg64::set_stream(stream) in order to get the same results from the PCG64 example in the parallel vignette when re-writing it while accessing the global RNG. See also Results from separate seed() and set_stream() differ from two-argument seed() imneme/pcg-cpp#91. This should have changed the stream of random numbers you are seeing in your tests, but your test cases are still working.

  3. In Feature/parallel generate #82 (merged in f2492c5 as version 0.3.2.6) I realized that the example was not working as expected. Instead we developed the parallel_generate template and there it was necessary for clone(0) to return an unchanged RNG, resulting in another change of how the two argument construction is done. Note, one should use current main or at least Add 'inline' to get_seed_from_r() #84 (merged in 2a18894) since an important inline was missing before that. This should have changed the stream of random numbers you are seeing in your tests, and now your test cases are no-longer working.

  4. In Restore pcg ctor #87 (not yet merged, version 0.3.2.7) I thought it should be possible to bring back the original interpretation of the two argument construction. This should have changed the stream of random numbers you are seeing in your tests back to the CRAN version, however, your test cases are still not working.

I am surprised by the success in 2. and especially by the failure in 4.

@jlmelville
Copy link

@rstub that is very perturbing. If I had to guess I would say I am doing something accidentally terrible like indirectly calling the R API from a thread or some kind of memory issue where the RNG state is being trampled on. I do test with valgrind/ABSAN/USAN but easy for something to slip through the cracks. I'll look more closely at that part of the code.

@rstub
Copy link
Member Author

rstub commented May 11, 2024

@joemsong: Thanks for the update to both GridOnClusters and FunChisq. Reverse dependency checks appear good again.

@rstub
Copy link
Member Author

rstub commented May 11, 2024

@jlmelville I have found the issue. In #87 I had made sure that random_64bit_wrapper<pcg64>::seed(seed, stream) was unchanged, but not dqrng::generator<pcg64>(seed, stream). The former is used by R, why you are using the latter. I have now fixed that, so that now the reverse dependency check for rnndescent appear good again.

@jlmelville
Copy link

Good news @rstub. Is there a branch/PR I can use to test on my side too?

@rstub
Copy link
Member Author

rstub commented May 11, 2024

@jlmelville #87 contains the current state of things. Thanks for testing!

@jlmelville
Copy link

@rstub I have tested with #87 and I can confirm tests are back to passing. So should be good to go from me. Thank you!

@rstub
Copy link
Member Author

rstub commented May 12, 2024

The most recent reverse dependency check looks good. I am closing this ticket and going to upload to package. Thanks everybody!

@s3alfisc fwildclusterboot is still strange. Sometimes it fails with the CRAN version, sometimes with the new version, sometimes with both (like in the last run), and sometimes not at all. Let's hope it works on CRAN like it is working there right now.

@rstub rstub closed this as completed May 12, 2024
@jlmelville
Copy link

@rstub I hate to be the bearer of... confusing news, but I was working on some bug fixes on rnndescent last night (not related to anything to do with random number generators) and added some tests to the master branch a few hours ago.

Unfortunately, when I run R CMD check on my master branch with #87 (and now the main branch of dqrng) I get a segfault during the test run, which I do not see with the CRAN release of dqrng.

I am not currently sure what is going on. I have reproduced the problem on Windows and Mac -- those are on R 4.4.0. On Linux (stuck on 4.3.1) I can't reproduce it (valgrind did not show me anything either). Even on Windows and Mac, it does not appear when I run devtools::test, only devtools::check -- but it does appear when running R CMD check from the command line on a source tarball so I can't discount it as some devtools/RStudio weirdness.

@rstub
Copy link
Member Author

rstub commented May 12, 2024

@jlmelville I will see if I can reproduce this (R 4.4 on Linux). Unfortunately, I have already submitted the package to CRAN.

@rstub
Copy link
Member Author

rstub commented May 12, 2024

@jlmelville I cannot reproduce this, but it looks like it is happening on CRAN :-( https://win-builder.r-project.org/incoming_pretest/dqrng_0.4.0_20240512_224437/reverseDependencies/

@s3alfisc Looks like fwildclusterboot is also failing on CRAN. Do you have any idea why it is behaving like this?

@achubaty Looks like SpaDES.tools is also affected. I am not sure why I have not seen that before, maybe because it only suggests dqrng?

@jlmelville
Copy link

@rstub thank you for the PR at jlmelville/rnndescent#16 I assume the way forward here would be to submit a new release of rnndescent?

@rstub
Copy link
Member Author

rstub commented May 13, 2024

@jlmelville Yes, thanks for taking care of it!

@achubaty
Copy link

achubaty commented May 13, 2024

I've implemented fixes (in PredictiveEcology/SpaDES.tools@44a71e3), most of which had to do with not properly resetting a seed between tests. Only one of the failures was caused by the update to the change in the dqrng RNGkind, where we were checking the values directly.

I'll submit an updated SpaDES.tools version to CRAN right away, once my changes all pass GitHub Actions checks.

@s3alfisc
Copy link

I have also set up a hotfix release and will merge as soon as the CI tests pass: s3alfisc/fwildclusterboot#151

@jlmelville
Copy link

@rstub I have submitted a new version of rnndescent to CRAN

@jlmelville
Copy link

@rstub rnndescent 0.1.6 is on CRAN and so far the CRAN checks look good

@achubaty
Copy link

@rstub SpaDES.tools 2.0.7 fixes the issue and was accepted by CRAN this morning.

@rstub
Copy link
Member Author

rstub commented May 15, 2024

Thanks all! dqrng 0.4.0 is now on CRAN. In the final reverse dependency checks there, fwildclusterboot was still failing, but we have a patch almost ready and rnndescent and SpaDES.tools looked good. Let's hope that will also apply for the other architectures that CRAN uses.

@rstub rstub closed this as completed Jul 25, 2024
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

No branches or pull requests

5 participants