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-1751 add safety #78

Merged
merged 5 commits into from
Oct 31, 2023
Merged

RXR-1751 add safety #78

merged 5 commits into from
Oct 31, 2023

Conversation

roninsightrx
Copy link
Contributor

@roninsightrx roninsightrx commented Oct 26, 2023

When duplicate names are used in covariate and declare_variables arguments, the covariates are not exported in the output column. Example to see this:

library(PKPDsim)
library(pkvoriconazolefriberg)
mod <- pkvoriconazolefriberg::model()
covs <- list(
  WT = new_covariate(70),
  AGE = new_covariate(70),
  CYP2C19unknown = new_covariate(1),
  CYP2C19a1a2 = new_covariate(0),
  CYP2C19a2a2 = new_covariate(0),
  CYP2C19a2a3 = new_covariate(0),
  CYP2C19a1a3 = new_covariate(0),
  CYP2C19a3a3 = new_covariate(0)
)
reg <- new_regimen(amt = 500, interval = 12, n = 5)
res <- sim(
  mod,
  covariates = covs,
  regimen = reg,
  parameters = pkvoriconazolefriberg::parameters(),
  output_include = list(variables = TRUE, covariates = TRUE)
)
res[1:5,]

After this change it's now impossible to specify duplicate covariates and variables in model specs. I initially implemented code in sim() that checks for duplicates in the output of sim() and made the parsing safer, but I removed that because it should now be unnecessary.

  • Added a test for throwing of the error
  • removed PKPDsim.Rproj (unrelated cleanup)

@roninsightrx roninsightrx marked this pull request as ready for review October 27, 2023 18:04
@roninsightrx roninsightrx requested a review from karawoo October 27, 2023 18:16
Copy link
Contributor

@karawoo karawoo left a comment

Choose a reason for hiding this comment

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

The code changes seems ok but I don't understand the reason behind removing the .Rproj file? Granted I don't use it since I don't use RStudio, but I thought most people do?

@roninsightrx
Copy link
Contributor Author

The code changes seems ok but I don't understand the reason behind removing the .Rproj file? Granted I don't use it since I don't use RStudio, but I thought most people do?

We can discuss, but IMO .Rproj files shouldn't be included, I sere it as more of a user/environment specific file rather than something specific to the package. In this case it was annoying because it stores a flag that controls compilation (if all C++ source files should be recompiled. The current version has that flag set to true, but it takes 30 secs or so extra so annoying when developing). I could've set it to false but decided to remove instead.

@karawoo
Copy link
Contributor

karawoo commented Oct 30, 2023

I think we should at least briefly discuss with the team to avoid breaking anyone's workflow for working with RStudio projects, because after this change RStudio's various project-specific options won't work (i.e. "open project" and "recent projects" won't work, you won't be able to double click on the .Rproj files to launch an RStudio session in that project, etc.).

Can we make it a separate PR from the RXR-1751 change?

@roninsightrx
Copy link
Contributor Author

I think we should at least briefly discuss with the team to avoid breaking anyone's workflow for working with RStudio projects, because after this change RStudio's various project-specific options won't work (i.e. "open project" and "recent projects" won't work, you won't be able to double click on the .Rproj files to launch an RStudio session in that project, etc.).

Can we make it a separate PR from the RXR-1751 change?

Sounds good. I mean the idea is still that everyone (that uses RStudio) keeps the Rproj file, just not committed to git. We'll discuss tomorrow.

@@ -17,5 +17,6 @@ StripTrailingWhitespace: Yes

BuildType: Package
PackageUseDevtools: Yes
PackageCleanBeforeInstall: No
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the setting I mentioned. For PKPDsim it's better if this is set to no, since we don't want to keep compiling the C++ code

@roninsightrx roninsightrx merged commit 3d214c9 into master Oct 31, 2023
1 check passed
@roninsightrx roninsightrx deleted the RXR-1751-fix-cov-var-dupl branch October 31, 2023 18:24
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