DO NOT MERGE: Converting PsN runs to bbr model objects

[barrettk]

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 processx to execute a PsN call of an existing bbr model, 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 ctl file in your terminal, and then run execute <mod_name>. After which, you should be able to identify which folders correspond to the example below.

# Test PsN converter ------------------------------------------------------

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"

.psn_mod_dir <- file.path(psn_dir, psn_type)
.psn_run_dir <- file.path(psn_dir, psn_type, "modelfit_dir1")
.bbr_mod_dir <- file.path(psn_dir, psn_type, "bbr_test") # wherever you want the bbr model files to be

mod <- convert_psn(.psn_mod_dir = .psn_mod_dir,
                   .psn_run_dir = .psn_run_dir,
                   .bbr_mod_dir = .bbr_mod_dir,
                   .overwrite = TRUE)

bbr::read_model(mod$absolute_model_path) # works

> mod

── Status ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

── Finished Running ──

── Absolute Model Path ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
• /data/Projects/package_dev/bbr/inst/model/nonmem/psn/mod_table/bbr_test/run105_table

── YAML & Model Files ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
• /data/Projects/package_dev/bbr/inst/model/nonmem/psn/mod_table/bbr_test/run105_table.yaml
• /data/Projects/package_dev/bbr/inst/model/nonmem/psn/mod_table/bbr_test/run105_table.ctl

> mod %>% model_summary()
Dataset: workshop.csv                                                                                                                                                                 

Records: 799     Observations: 760   Subjects: 40

Objective Function Value (final est. method): 2604.001

Estimation Method(s):

– First Order Conditional Estimation with Interaction 

No Heuristic Problems Detected

|parameter_names |estimate |stderr |shrinkage |
|:---------------|:--------|:------|:---------|
|THETA1          |2.31     |0.0870 |          |
|THETA2          |78.1     |3.19   |          |
|THETA3          |462      |29.4   |          |
|THETA4          |-0.0797  |0.0552 |          |
|THETA5          |4.12     |1.35   |          |
|THETA6          |0.559    |0.0444 |          |
|OMEGA(1,1)      |0.0199   |0.0115 |38.6      |
|OMEGA(2,2)      |0.150    |0.0264 |1.77      |

> mod %>% model_summary() %>% get_param()
$omega                                                                                                                                                                                
          OMEGA_1  OMEGA_2
OMEGA_1 0.0199237 0.000000
OMEGA_2 0.0000000 0.149606

$sigma
        SIGMA_1
SIGMA_1       1

$theta
     THETA1      THETA2      THETA3      THETA4      THETA5      THETA6 
  2.3129600  78.1438000 462.0470000  -0.0796566   4.1173400   0.5586930 

> mod %>% model_summary() %>% param_estimates()
# A tibble: 10 × 8                                                                                                                                                                    
   parameter_names estimate   stderr random_effect_sd random_effect_sdse fixed diag  shrinkage
   <chr>              <dbl>    <dbl>            <dbl>              <dbl> <lgl> <lgl>     <dbl>
 1 THETA1            2.31   8.70e- 2           NA              NA        FALSE NA        NA   
 2 THETA2           78.1    3.19e+ 0           NA              NA        FALSE NA        NA   
 3 THETA3          462.     2.94e+ 1           NA              NA        FALSE NA        NA   
 4 THETA4           -0.0797 5.52e- 2           NA              NA        FALSE NA        NA   
 5 THETA5            4.12   1.35e+ 0           NA              NA        FALSE NA        NA   
 6 THETA6            0.559  4.44e- 2           NA              NA        FALSE NA        NA   
 7 OMEGA(1,1)        0.0199 1.15e- 2            0.141           4.09e- 2 FALSE TRUE      38.6 
 8 OMEGA(2,1)        0      1   e+10            0               1   e+10 TRUE  FALSE     NA   
 9 OMEGA(2,2)        0.150  2.64e- 2            0.387           3.42e- 2 FALSE TRUE       1.77
10 SIGMA(1,1)        1      1   e+10            1               1   e+10 TRUE  TRUE       3.34

[barrettk]

@seth127

Current concerns:

• multiple modelfit_dir's
  • wouldn't always be the max --> could be a bootstrap or some other method where multiple fits occur.
• multiple NM_run's
  • Multiple folders are generated if the model fails to converge for parameter estimates. I think it can happen for other intentional scientific reasons as well though.
• PsN calls outside of execute (i.e. bootstrap, scm, etc.)
• The threads attribute I pulled from the PsN generated meta.yaml was wrong. It said I had 5 threads, when A) I never specified any (should be 1), and B) I only had 4 cores for my workflow. However, its possible this is the default value, and it wasn't actually used since it wasn't parallelized, which brings me to the next point...
• Couldnt find a setting for whether the model was run in parallel or not.
• Some parameters were not retrievable to match a legitimate bbi json file
  • The way I handled the nonmem chunk of the bbi json may not be good enough
• We should probably append the json with a parameter indicating whether the model was run via PsN or not. We could then use this parameter to prevent certain functions from working (if there's no workaround). Would need to have a deeper discussion about appending the bbi json though; just in case it has other implications im not aware of.
• Did I copy over everything that could be necessary?
  • We could look at this multiple ways. Do we want to pull over only whats necessary for model_summary() and other functions to run, or do we want PsN generated RMD's to still run? Are there other files that aren't required, but the modeler would want to keep?
• Other assumptions I mentioned in inline code comments

[seth127]

Good start here. I'll leave all the bbi_config.json stuff alone for now, though we definitely need to get back to it. Going to concentrate on the interface and how/where we get the files that we're copying.

Interface

Assuming 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 .psn_model_file = NULL (default) look for the run name in {.modelfit_dir}/command.txt and look for the file in dirname(.modelfit_dir). If we don't find that file, error and ask them to pass a path to .psn_model_file.

If we do find it, copy it over (but don't rename it). We need to look into the $DATA issue, but consider if we want to look for the file at $DATA and warn if we can't find it. (more on this below)

Where to get files

• Need to clarify with users, but for now, copy from max NM_run dir (like you're doing)
• Copy everything that we want from NM_run/psn.* (i.e. don't try to copy stuff from top-level renamed files)
• Same thing ^ with tables (which I think is what you're doing

Dealing with $DATA

Context

bbi assumes the path in $DATA is relative to a directory one level deeper than the control stream file, because this is where it actually executes the model. However, for .mod files it does not assumes this and will actually add an ../ to the path in the control stream that gets copied through to the output/execution directory. This was based on a PsN-related assumption. Details in code:

• copyFileToDestination() function call that passes strings.ToLower(l.Extension) == "mod" to modifyPath arg. (In other words: if extension is .mod, then modifyPath = TRUE)
• modify_path control flow that calls AddPathLevelToData().
• AddPathLevelToData() code that adds an extra ../

Ramifications

• When copying the .mod control stream, we don't change the$DATA path in the original .mod file, but we do need to add ../ to the $DATA path in the .mod that is copied into the output/execution dir.
  • This is to imitate what bbi would've done if it ran this model
• If .bbr_dir is not dirname(.modelfit_dir) (aka the proposed default), then we should probably modify the data path to be relative to the new location of the control stream. This might take some fs::path_rel() gymnastics.
• As mentioned above, I also think we should check (when we're doing the conversion/copy) whether we can actually find this data file. If not, let's just give a warning saying "expected to find data at {path we parsed} but no file is there. Please adjust $DATA in {path to new control stream}."

bbi_config.json data path

We also need to but the (modified) data path into data_path: in bbi_config.json. One reason for this is that get_data_path() will look for it there (and this called by things like nm_join() and friends).

I know I said we'd talk more about bbi_config.json later, but while we're here, let's make data_md5 and model_md5 something like "PRERUN". We're going to want those to be an unequivocal string that we can use to short-circuit other helpers that try to use them.

[barrettk]

We also need to but the (modified) data path into data_path: in bbi_config.json. One reason for this is that get_data_path() will look for it there (and this called by things like nm_join() and friends).

I know I said we'd talk more about bbi_config.json later, but while we're here, let's make data_md5 and model_md5 something like "PRERUN". We're going to want those to be an unequivocal string that we can use to short-circuit other helpers that try to use them.

Just FYI this is already done in convert_psn_data_path. I've tested that already and that seems to concistently deliver the right path, depending where the new bbr dir was set to. The "prerun" aspect hasn't though. Might need some more context for that

[barrettk]

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)

[barrettk]

Just realized I cant take the max of a folder (like I was doing for NM_run directories) because they use literal sorting rather than numeric:

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)))]
  }

[barrettk]

@seth127 it seems the pirana examples dont have a meta.yaml file, which was introduced in PsN 4.8.0

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 meta.yaml dependency given all the submission info it captures, but open to suggestions. Will see what other options we have.

Edit

I 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 (psn -version) to decide what to look for. As an aside, the text file doesnt contain all the same information, so not 100% sure if this will work

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.mod

This 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:

• if PsN is >= 4.8.0 --> use yaml. If .psn_model_file is NULL we can grab the model file used (unformatted/messed up model file)
• If PsN <= 4.8.0 --> use text file. If .psn_model_file is NULL, we have to use the model file in the NM_run folder, which is formatted to have less white space and is less readable

[barrettk]

@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.

[kyleam]

@barrettk Hmm, that is a bizarre failure. From drone:

+ git fetch origin +refs/heads/main:
From https://github.com/metrumresearchgroup/bbr
 * branch            main       -> FETCH_HEAD
 * [new branch]      main       -> origin/main
+ git checkout main
Branch 'main' set up to track remote branch 'main' from 'origin'.
Switched to a new branch 'main'
+ git fetch origin refs/pull/568/head:
From https://github.com/metrumresearchgroup/bbr
 * branch            refs/pull/568/head -> FETCH_HEAD
+ git merge d4f5b5ef1d498904dc19886141d065ce88634bb4
merge: d4f5b5ef1d498904dc19886141d065ce88634bb4 - not something we can merge

Nothing there looks strange, aside from the failure message of course, and the commit is something it can merge:

$ git checkout origin/main
Previous HEAD position was d4f5b5ef Bug fix: now properly finds the latest NM_run folder
HEAD is now at 89f9f930 Merge pull request #567 from metrumresearchgroup/release/1.5.0
$ git fetch origin refs/pull/568/head:
From github.com:metrumresearchgroup/bbr
 * branch              refs/pull/568/head -> FETCH_HEAD
$ git rev-parse FETCH_HEAD
d4f5b5ef1d498904dc19886141d065ce88634bb4
$ git merge FETCH_HEAD
Updating 89f9f930..d4f5b5ef
Fast-forward
 NAMESPACE                    |   1 +
 R/convert-psn.R              | 314 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 man/convert_psn.Rd           |  50 ++++++++++++++
 man/convert_psn_data_path.Rd |  17 +++++
 man/copy_psn_run_files.Rd    |  27 ++++++++
 man/copy_psn_tables.Rd       |  24 +++++++
 man/create_psn_json.Rd       |  17 +++++
 7 files changed, 450 insertions(+)
 create mode 100644 R/convert-psn.R
 create mode 100644 man/convert_psn.Rd
 create mode 100644 man/convert_psn_data_path.Rd
 create mode 100644 man/copy_psn_run_files.Rd
 create mode 100644 man/copy_psn_tables.Rd
 create mode 100644 man/create_psn_json.Rd

$ git log --oneline origin/main..d4f5b5ef1d498904dc19886141d065ce88634bb4
d4f5b5ef (HEAD, origin/feat/PsN_output_helper, refs/pull/origin/568) Bug fix: now properly finds the latest NM_run folder
6668585b refactored conversion to use better arguments
aedeeba5 updated .mod_path variable definition for several helper functions
abfee5c8 added clarification comment
d3af42a0 incomplete comment
9160b12d PsN converter is now functional (with a lot of assumptions)
e1a71fc5 Psn output conversion (WIP)

Does it persist if you restart?

[barrettk]

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?

[kyleam]

Restart my R session?

The drone build, via the button on the right:

[barrettk]

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 meta.yaml and/or version_and_option_info.txt file to reference file paths that do not exist on the system.

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:

• if PsN is greater than 4.8.0 (if theres a yaml file), we will grab the data path described there (accurate if run on the same machine)
• If PsN is less than 4.8.0 (there's no yaml), we will grab the directory from the text file (modelfit_dir), and check to see if the model file is there. If not, error out and prompt the user to specify a path. We could grab the psn.mod, but that has the aforementioned formatting issues.

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.

[seth127]

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

I added a bunch of handling/guessing for model paths...

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 . psn_model_file. I don't think we want to grab the badly formatted one in the run dir. If they really want to use that, then they can pass that path manually.

data paths have not been handled yet... we have the option of either A) erroring out if we cant find it, or B) allowing the user to specify a datapath

I think we should basically do a two-step thing:

1. try to find the model file (error if we can't, as we do now)
2. parse the $DATA from that and then search for the data file from there. That path will be relative to the location of the model file that we just found. If we can't find it, let's give a warning saying "expected to find data at {path we parsed} but no file is there. Please adjust $DATA in {path to new control stream}." I don't think we actually need to error (though we'll want to get user feedback on this)

One more consideration, copied from above, when dealing with the data path:

• If .bbr_dir is not dirname(.modelfit_dir) (aka the proposed default), then we should probably modify the data path to be relative to the new location of the control stream. This might take some fs::path_rel() gymnastics.

Basically we want to be sure that, if we found the data file, the new copied .mod file still has the right path to it.

Tangent about .mod file in execution dir

One last note... I had written this above:

When copying the .mod control stream, we don't change the $DATA path in the original .mod file, but we do need to add ../ to the $DATA path in the .mod that is copied into the output/execution dir. This is to imitate what bbi would've done if it ran this model.

but now I'm thinking... do we even need to copy in the .mod file to the execution directory? Do we use that for anything, once execution is finished? I'm thinking we might want to try not copying it and see if anything breaks. It just seems... unnecessary and confusing. (To be clear, I know we need this, I'm talking about getting rid of this.)

[barrettk]

One last note... I had written this above:

When copying the .mod control stream, we don't change the $DATA path in the original .mod file, but we do need to add ../ to the $DATA path in the .mod that is copied into the output/execution dir. This is to imitate what bbi would've done if it ran this model.

but now I'm thinking... do we even need to copy in the .mod file to the execution directory? Do we use that for anything, once execution is finished? I'm thinking we might want to try not copying it and see if anything breaks. It just seems... unnecessary and confusing. (To be clear, I know we need this, I'm talking about getting rid of this.)

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?

[seth127]

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:

• We want these to function as if run by bbr, but in terms of consistency, I think it's probably beneficial if it's obvious that this actually wasn't run with bbr, and so some of the reproducibility promises aren't the same. In that sense, some inconsistency might be semi-desirable, because it just makes that more obvious to future users.
• This copied control stream is really just an artifact of how bbi executes. Similar to previous comment: missing it just shows that it wasn't run with bbi.
• The copied control stream isn't necessary for re-running the model. In fact, if the user decides to re-run the model, it would get overwritten anyway.

[barrettk]

• This copied control stream is really just an artifact of how bbi executes. Similar to previous comment: missing it just shows that it wasn't run with bbi.

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 .mod vs .ctl extensions have slightly different file paths, where one points to the run directory and the other points to the original model. Ill have to look further into that to make sure the constructed data paths are still correct (since they're relative paths), as that would matter if the user chose to re-run the model with bbr (though not sure how likely that is)

[seth127]

Testing this and it's looking good. Some small changes.

• [x] .data_path in the bbi_config.json should be relative or else get_data_path() breaks. This line should be data_path = fs::path_rel(.data_path, .bbr_run_dir),
• [x] Don't add .tab to the table files. Copy them as is.
• [ ] Warn instead of error when we can't find the table files referenced in the control stream
• [x] Make .bbi_dir a required argument

finish example

Once that's done, let's

• update this line, and the one a few lines down, in the example script.
• test that the example script all runs without error
• tag it 1.5.0.8002

[seth127]

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.).