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

RXR-2140: fix issue due to change in warning behavior in upcoming Rcpp #103

Merged
merged 3 commits into from
Jun 24, 2024

Conversation

roninsightrx
Copy link
Contributor

@roninsightrx roninsightrx commented Jun 24, 2024

See description of change in Rcpp.

The failure in PKPDsim test was due to an erroneously configured test case, which had the wrong number of bins for simulation of inter-occasion variability. This led to extra warnings using the new Rcpp. This PR fixes the test case.

It also adds an extra warning to make it harder to misconfigure the number of IOV bins (and a new test to confirm the warning is thrown). I did not make that a hard stop(), because in most cases the misconfiguration is not affecting the results.

For reviewing this PR:

  1. first run tests using old version of Rcpp (anything before 1.0.12.4). The tests should all run fine, either on master or on this branch.
  2. then install latests version on Rcpp/main using remotes::install("RcppCore/Rcpp"). Then rerun tests. The tests should fail on the PKPDsim/master branch, but not on PKPDsim/RXR-2140-Rcpp-issue.

Copy link
Contributor

@jasmineirx jasmineirx left a comment

Choose a reason for hiding this comment

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

this looks good to me, but does it make sense for this to be a warning rather than a stop? if they are mismatched, something was misspecified by the user

@roninsightrx
Copy link
Contributor Author

this looks good to me, but does it make sense for this to be a warning rather than a stop? if they are mismatched, something was misspecified by the user

I was going back and forth on that, but I think a warning is fine.

@roninsightrx roninsightrx merged commit 1de20fb into master Jun 24, 2024
3 checks passed
@roninsightrx roninsightrx deleted the RXR-2140-Rcpp-issue branch June 24, 2024 19:47
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