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

Numerical stability of beta_mix_trunc_lpdf #62

Merged
merged 2 commits into from
Apr 16, 2023

Conversation

andrjohns
Copy link
Contributor

@andrjohns andrjohns commented Apr 2, 2023

This PR updates your package's tests to temporarily skip one of the scale factors when testing. This was causing a test failure due to a bug in Stan 2.26, and will be safe to re-add on the release of rstan 2.31 (which will quickly follow the release of rstan 2.26).

I've also updated your package's NAMESPACE to avoid a NOTE about not importing from rstantools.

If you could submit a new release to CRAN soon with this PR, it would be very much appreciated - we're trying to reduce the number of downstream failures so that we can submit

@andrjohns andrjohns requested a review from kkmann as a code owner April 2, 2023 14:50
@kkmann
Copy link
Collaborator

kkmann commented Apr 3, 2023

will look into it today/tomorrow (and the mac fail x/) - could you give me a link to the stan bug for reference?

@andrjohns
Copy link
Contributor Author

Unfortunately I haven't been able to easily reproduce it to be able to identify specifically where in Stan the issue is coming from, just that it is present with rstan 2.26 but not 2.31. Additionally, the failure only occurs when the tests are run through rcmdcheck(args="--as-cran"), not when the tests are run manually or through testthat.

If you have any suggestions for the different behaviours under rcmdcheck vs testthat, I could look into more, but given that it's resolved by 2.31 I'm not too concerned

If you want to verify this locally as well, you can compare the results of running rcmdcheck() under 2.26:

install.packages("rstan", repos = c("https://mc-stan.org/r-packages/", getOption("repos")))

Against the results from 2.31:

remotes::install_github("stan-dev/rstan@experimental",subdir="StanHeaders")
remotes::install_github("stan-dev/rstan@experimental",subdir="rstan/rstan")

@andrjohns
Copy link
Contributor Author

While I still don't know exactly what caused it, I narrowed the issue down to a loss of precision in your beta_mix_trunc_lpdf function. By updating the function to operate entirely on the log-scale, the tests pass under all versions of rstan.

Hopefully that's a better solution for you than just removing tests!

@andrjohns andrjohns changed the title Update test case Numerical stability of beta_mix_trunc_lpdf Apr 5, 2023
@andrjohns
Copy link
Contributor Author

@kkmann apologies to double-ping, but we're looking to make the submission soon. Would it be much trouble to merge and release this fix? Thanks!

Copy link
Collaborator

@kkmann kkmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh wow, cool - thanks!

@kkmann kkmann merged commit f397a9c into Boehringer-Ingelheim:main Apr 16, 2023
@kkmann
Copy link
Collaborator

kkmann commented Apr 17, 2023

@andrjohns on its way to CRAN - there was/is a weird bug with ggplot2 on Mac and I do not have a Mac testing environment... fingers crossed.

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

Successfully merging this pull request may close these issues.

2 participants