DO NOT MERGE: Converting PsN runs to bbr model objects #568
Conversation
Current concerns:
|
|
Good start here. I'll leave all the InterfaceAssuming there is only one mod file in the directory won't work, because people often have many different runs in the same dir. Consider this interface instead: convert_psn(
.modelfit_dir, # no default, this will be the path to a modelfit_dir
.bbr_dir = dirname(.modelfit_dir), # default to the same dir that modelfit_dir is in
.psn_model_file = NULL
)To find the original control stream, if If we do find it, copy it over (but don't rename it). We need to look into the Where to get files
Dealing with $DATAContext
Ramifications
bbi_config.json data pathWe also need to but the (modified) data path into I know I said we'd talk more about |
Just FYI this is already done in |
|
Using new method: psn_dir <- system.file(file.path("model", "nonmem", "psn"), package = "bbr", mustWork = TRUE)
psn_dir <- fs::path_rel(psn_dir, getwd())
psn_type <- "mod_table" # mod_table, mod, or ctl
.modelfit_dir <- file.path(psn_dir, psn_type, "modelfit_dir1")
.bbr_dir <- file.path(psn_dir, psn_type, "bbr_test")
.psn_model_file <- file.path(psn_dir, psn_type, "run105_table.mod")
mod <- convert_psn(.modelfit_dir = .modelfit_dir,
.bbr_dir = .bbr_dir,
.overwrite = TRUE)
mod <- convert_psn(.modelfit_dir = .modelfit_dir,
.psn_model_file = .psn_model_file,
.bbr_dir = .bbr_dir,
.overwrite = TRUE) |
|
Just realized I cant take the max of a folder (like I was doing for psn_fit_dirs <- psn_fit_dirs[grepl("modelfit", psn_fit_dirs)] %>% sort()
> psn_fit_dirs
[1] "/data/pirana_examples/modelfit_dir1" "/data/pirana_examples/modelfit_dir10" "/data/pirana_examples/modelfit_dir11"
[4] "/data/pirana_examples/modelfit_dir12" "/data/pirana_examples/modelfit_dir13" "/data/pirana_examples/modelfit_dir14"
[7] "/data/pirana_examples/modelfit_dir2" "/data/pirana_examples/modelfit_dir3" "/data/pirana_examples/modelfit_dir4"
[10] "/data/pirana_examples/modelfit_dir5" "/data/pirana_examples/modelfit_dir6" "/data/pirana_examples/modelfit_dir7"
[13] "/data/pirana_examples/modelfit_dir8" "/data/pirana_examples/modelfit_dir9"
> max(psn_fit_dirs)
[1] "/data/pirana_examples/modelfit_dir9"Fix:From: if(length(.psn_run_dir) > 1){
.psn_run_dir <- max(.psn_run_dir))]
}To: if(length(.psn_run_dir) > 1){
.psn_run_dir <- .psn_run_dir[which.max(readr::parse_number(basename(.psn_run_dir)))]
} |
|
@seth127 it seems the pirana examples dont have a Given that we're on 5.2.6, we must have executed these models a long while back. Id really prefer not to remove the EditI found additional text files that may contain what we need, but will have to write a helper function to parse the text file or yaml. Not sure if I should check for the presence of a yaml file, or check the installed PsN version ( The main difference im seeing is that the yaml has the location of the model file used during the execution call, while the text file does not: model_files:
- /data/Projects/package_dev/bbr/inst/model/nonmem/psn/mod_table/run105_table.modThis is unfortunate, as I haven't found a way to map back to the model used without the yaml file. It seems like ill have to use the logic:
|
|
@kyleam have you seen anything like the most recent drone failure in this PR? Seems like it failed during cloning with a bizarre message. Not the first time i've seen this. |
|
@barrettk Hmm, that is a bizarre failure. From drone: Nothing there looks strange, aside from the failure message of course, and the commit is something it can merge: Does it persist if you restart? |
Restart my R session? I dont have any issues locally. I bet my next commit won't run into the drone issue too (just a hunch), but I dont see what restarting would do. Are you suggesting restarting before my next commit? |
The drone build, via the button on the right:
|
- Discovered we will likely need additional handling for model -and- data paths in the event the model was executed on a different system, and then just copied over. I added a bunch of handling/guessing for model paths, but data paths have not been handled yet. Pirana examples wont work without allowing users to specify a datapath as well (or we could just error out)
|
FYI @seth127 I discovered we will likely need additional handling for model and data paths in the event the model was executed on a different system, and then just copied over. This causes the I added a bunch of handling/guessing for model paths, but data paths have not been handled yet. The pirana examples wont work without allowing users to specify a datapath as well (or we could just error out). At the moment, if no model file is specified, we will do one of two things:
To handle datapath discrepancies however, we only have the option of either A) erroring out if we cant find it, or B) allowing the user to specify a datapath (would only ever be used for models run on a different machine). In all honesty I dont like either solution, but would lean towards option B. If we chose option A), all the model_path handling I just implemented would pointless, since we'd error out as soon as we recognized some file paths didnt exist. |
|
This is all looking good @barrettk. Overall, I think we should make a few more tweaks and then reach out to some users to test is out and give feedback. Discussion on those below: Path answers
This sounds good. I think we want to make best effort to find the original model file, but if we can't then we just need to error and tell the user to pass it in via
I think we should basically do a two-step thing:
One more consideration, copied from above, when dealing with the data path:
Basically we want to be sure that, if we found the data file, the new copied Tangent about
|
To my knowledge, no we dont need it (will confirm), but I mostly did that because previous bbr runs have multiple copies of the model, and I figured we would want the execution to look as similar as possible. Will confirm the functional aspect, but what are your thoughts on keeping it just for consistency's sake? |
At this point I'm thinking we should not copy it, unless we need it, for the following reasons:
|
I definitely agree with it being desirable that there are obvious differences. Main reason Im nervous of downstream effects is because we had talked about how |
…r it was in a subdirectory, or outer directory. Introducing a WIP commit to store some of the commented out changes
- Have to test if we want the bbr run directory or initial bbr_dir for finding the relative data path stored in the ctl file (i.e. test model submission). - Have to confirm if .mod or .ctl extensions change this
- we now check multiple locations for a model (informed possibilities) - Fixed a bug that would prevent model name detection if command args were issued in the PsN call - discovered modelfit_dir2 wont work because its missing its table files - modelfit_dir9 wont work (currently) because multiple models were submitted at once. If we want this to work, we need an alternative/more reliable way to find the model name from the execution text file.
- data paths set properly in model - ctl vs mod extensions handled properly - multiple simultaneous model submissions are now handled, but a model file must be specified - both .mod and .ctl extensions can be submitted. This was tested with both the non-commited internal PsN models, as well as with the Pirana-examples
|
Testing this and it's looking good. Some small changes.
finish exampleOnce that's done, let's
|
| # this is needed to create a fake bbi json, and we dont want to copy everything just to fail at the end | ||
| # is_empty wont capture all cases - revisit how to properly address this concern | ||
| if(rlang::is_empty(bbi_version())){ | ||
| if(!nzchar(bbi_version())){ |
There was a problem hiding this comment.
maybe I'm missing something but do they actually need to have bbi installed to run this? They'll need it to do model_summary(), which is a likely next step. But I don't think we actually need bbi for this function, right?
| @@ -193,7 +182,7 @@ get_psn_submission_info <- function(.modelfit_dir, .psn_model_file = NULL){ | |||
| #' | |||
| #' @keywords internal | |||
| parse_psn_submission_info <- function(.submission_file, .psn_model_file){ | |||
| # browser() | |||
| # browser() | |||
There was a problem hiding this comment.
delete this browser() call
| @@ -54,7 +54,10 @@ mod_sum %>% get_theta() | |||
| # one of the nice things about bbr is how it integrates with other MeRGE packages | |||
| # here we show how to make simple diagnositic plots with the pmplots package | |||
| # see more here: https://merge.metrumrg.com/expo/expo1-nonmem-foce/posts/intro-to-pmplots.html | |||
| if (!nzchar(packageVersion("pmplots"))) remotes::install_github("metrumresearchgroup/pmplots") | |||
| if(require(pmplots)){ | |||
There was a problem hiding this comment.
Using require() is fine, but you have the logic backwards. This is saying to install if it's already installed.
You want the following (without my comments)
if (!require(pmplots)) { # load the library, if not installed `!require` will return TRUE
remotes::install_github("metrumresearchgroup/pmplots") # install
library(pmplots) # load the library, now that it's installed
}
There was a problem hiding this comment.
Ahh good catch. Yeah I found that nzchar returned an error for me. Didnt look into it too much, but with this fix I think it will be more reliable
…tion handling for bbr_dir (not allowed)
|
This looks good. As stated previously, we're not going to merge this now because we want to get some user feedback (and write some formal tests, etc.). |
seth127
changed the title
seth127
changed the title
Keeping this PR open for now, while we collect user feedback on this function...
Example
Can be tested using the code below. However at this point in time, the new PsN models/data have not been committed to this branch. In the end we should be able to use
processxto execute a PsN call of an existingbbrmodel, though I think we will likely want to add additional models to ensure our testing is comprehensive (there's a lot more variability in PsN runs)If anyone wants to test this with existing bbr models, copy a model to a new location, navigate to the
ctlfile in your terminal, and then runexecute <mod_name>. After which, you should be able to identify which folders correspond to the example below.