-
Notifications
You must be signed in to change notification settings - Fork 8
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
Comments
@rstub thanks for the heads-up, I'll take a look at this as soon as I can |
@jlmelville Great! The following patch works for me both with the CRAN version and the version from #79:
Feel free to use it. I can also provide a PR for it. |
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 FunChisq
fwildclusterboot
greeks
|
@rstub is the current state of
Also: yes I would be happy to see if I can stop using |
@jlmelville Please use the state from #79 for testing. Current main is missing an important Concering |
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?
Understood. |
@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. |
@s3alfisc: I was able to reproduce the issue with the CRAN version 0.13 of |
@joemsong: I was able to reproduce the issue with the CRAN versions of both |
@ahudde: I was able to reproduce the issue with the CRAN version of |
Hi @rstub , I have no immediate plans for a CRAN release - would you like me to submit a hotfix to 0.13 by adding |
Dear @rstub, |
Just to confirm @rstub, as #79 is complete, would it now be helpful to submit a new version of |
@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. |
🎉 Latest revdep check no longer shows a failure with |
@s3alfisc: It is odd, that
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. |
@jlmelville This is strange. I am currently seeing reverse dependency failures from |
@rstub by building off the current 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 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. |
@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
I am surprised by the success in 2. and especially by the failure in 4. |
@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. |
@joemsong: Thanks for the update to both GridOnClusters and FunChisq. Reverse dependency checks appear good again. |
@jlmelville I have found the issue. In #87 I had made sure that |
Good news @rstub. Is there a branch/PR I can use to test on my side too? |
@jlmelville #87 contains the current state of things. Thanks for testing! |
The most recent reverse dependency check looks good. I am closing this ticket and going to upload to package. Thanks everybody! @s3alfisc |
@rstub I hate to be the bearer of... confusing news, but I was working on some bug fixes on Unfortunately, when I run 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 |
@jlmelville I will see if I can reproduce this (R 4.4 on Linux). Unfortunately, I have already submitted the package to CRAN. |
@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 @achubaty Looks like |
@rstub thank you for the PR at jlmelville/rnndescent#16 I assume the way forward here would be to submit a new release of |
@jlmelville Yes, thanks for taking care of it! |
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 I'll submit an updated |
I have also set up a hotfix release and will merge as soon as the CI tests pass: s3alfisc/fwildclusterboot#151 |
@rstub I have submitted a new version of |
@rstub |
@rstub |
Thanks all! |
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 infoNewly broken
@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 infoNewly broken
Newly fixed
Installation
Devel
@jlmelville This is clearly my redefinition of
dqrng::rng64_t
. It might make sense to usedqrng::generator<pcg64>()
instead. I can provide a PR for that. BTW, given that I have factored out the sampling code intodqrng_sample.h
: Do you still need your own copydqsample.h
?The text was updated successfully, but these errors were encountered: