update_model_id mishandles based_on field

[kyleam]

The goal of update_model_id is to replace parent model ID with the new model ID. It gets the parent ID directly from the based_on field:

https://github.com/metrumresearchgroup/bbr/blob/727b65b5cfba817a49d29d290ccc5c47ead050e8/R/copy-model-helpers.R#L68

based_on is the relative path from the YAML, which means it can be something like ../1100. update_model_id mishandles this in two ways:

• it doesn't account for based_on being a path that only corresponds to the model ID when the parent is in the same directory as the child model.

  Two potential fixes:

  • take the basename of the based_on field
  • use get_model_id on the based_on value (get_model_id(get_based_on(...)))

  I like the second approach because it more clearly communicates the desired result, and it lets the method worry about the implementation details (though it's unlikely we'll ever have a model type that breaks the basename -> model ID mapping).

• it uses the value as a regular expression pattern without protecting it as literal value (e.g., with \\Q...\\E)

example of bad update

Here's an example of a bad update: ``` $ tail -n3 inst/model/nonmem/bayes/{1100/1100-1.yaml,1100.ctl,1100/1100-1.ctl} ==> inst/model/nonmem/bayes/1100/1100-1.yaml <== description: Chain 1 based_on: - ../1100 ==> inst/model/nonmem/bayes/1100.ctl <== $EST METHOD=BAYES CTYPE=0 SEED=1 NBURN=10 NITER=50 PRINT=10 MSFO=./1100.msf RANMETHOD=P PARAFPRINT=10000 BAYES_PHI_STORE=1 $TABLE NUM CL V2 Q V3 KA ETAS(1:LAST) EPRED IPRED NPDE EWRES NOPRINT ONEHEADER FILE=1100.tab RANMETHOD=P ==> inst/model/nonmem/bayes/1100/1100-1.ctl <== $EST METHOD=BAYES CTYPE=0 SEED=1 NBURN=10 NITER=50 PRINT=10 MSFO1100-1.msf RANMETHOD=P PARAFPRINT=10000 BAYES_PHI_STORE=1 $TABLE NUM CL V2 Q V3 KA ETAS(1:LAST) EPRED IPRED NPDE EWRES NOPRINT ONEHEADER FILE=1100.tab RANMETHOD=P ``` Note that 1) `MSFO=./1100.msf` is updated to `MSFO1100-1.msf` due to the unprotected `..` 2) the `FILE=1100.tab` value isn't updated (because it doesn't match `../1100{suffixes}`). (The above is from bbr.bayes, but that's likely going to move away from `update_model_id` for other reasons, so this issue isn't blocking anything)

Note that any fix here will end up conflicting with @barrettk's gh-605. However, at least based on the discussion so far, that PR will end up being closed without merge; in my opinion it's fine to move ahead with fixing this issue based on mains current code.

cc @seth127

[seth127]

That all makes sense to me @kyleam, nice catch.

I like the second approach... use get_model_id on the based_on value (get_model_id(get_based_on(...)))

@barrettk would you mind putting a PR to fix this when you have a chance?