update_model_id mishandles based_on field
Closed this issue · 1 comments
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:
Line 68 in 727b65b
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_onbeing 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
basenameof thebased_onfield - use
get_model_idon thebased_onvalue (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 IDmapping). - take the
-
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