Add compartments cmt to regimens in nm_to_regimen#128
Conversation
|
It will be great to have this functionality available, and the testing looks good for this approach. I have one question about the design choice. I see you highlighted this as a breaking change. Is there a way it could be elegantly designed such that it is not a breaking change? For example, in It might also be a good idea to move cmt_mapping to be the final argument so that old pass-by-position argument calls are not broken. |
jasmineirx
left a comment
There was a problem hiding this comment.
in general lgtm, a couple suggestions for clean-up. what about a case where dose_cmts is defined but the data doesn't have CMT?
R/nm_to_regimen.R
Outdated
| } | ||
| if(type == "infusion") { | ||
| reg[[i]] <- new_regimen(amt = tmp$amt, times = tmp$time, type = "infusion", t_inf = tmp$amt / tmp$rate) | ||
| if (!is.null(dose_cmts)){ |
There was a problem hiding this comment.
you might also want to add a check for !is.null(tmp$cmt) since the code below uses it.
R/nm_to_regimen.R
Outdated
| reg[[i]] <- new_regimen( | ||
| amt = tmp$amt, | ||
| times = tmp$time, | ||
| cmt = tmp$cmt, |
There was a problem hiding this comment.
I think we could skip this argument. if it is null, PKPDsim::sim will use the compartment definitions of the model file, which is probably safer.
There was a problem hiding this comment.
I can see this leading to a situation where you specify CMT = 1 but PKPDsim keeps giving you CMT = 2 and you have no idea where it is coming from. Explicit is better than implicit, and this function is basically not used anywhere except in PKPDposterior (which is essentially unmaintained) and in mipdeval, where the original fix would have already worked.
There was a problem hiding this comment.
Could you help me understand this case with an example? It is possible that NONMEM-style data and PKPDsim model definitions use different compartments for one dose type.
There was a problem hiding this comment.
https://github.com/InsightRX/mipdeval/pull/25/files
This is the only relevant place this function is currently being used, where we want to specify which compartments are from which doses using the NONMEM data set. Leaving it to be assumed by the model downstream, especially when CMT should be specified in the data set anyway, is not optimal imo.
|
I have completely overhauled this PR. As discussed with @roninsightrx , we are going to rely on CMT being passed into PKPDsim::regimen() and having that be interpreted correctly by the model. First, I had to check that passing compartment and type in new_regimen will result in compartment being respected by the simulator. For voriconazole Friberg, oral is compartment 1 and IV is compartment 2. The plot shows the cmt is ultimately what matters for the simulation.
Next, I checked whether this function nm_to_regimen works in the same way. Here, we give CMT with and without RATE, though in any data set with mixed oral/infusion dosing, you will have RATE. |
| ) | ||
| }) | ||
| } | ||
| } else if ("rate" %in% colnames(doses) &! 0 %in% doses$rate){ |
There was a problem hiding this comment.
What if we have two doses: on a first dose RATE=0 (bolus loading) and on second dose RATE=1? Then all doses will be treated as bolus, I think. Would add that as a test case to check, and potentially update behavior?
There was a problem hiding this comment.
The two blocks below are for backwards compatibility. If you have CMT defined (which you really should), then the previous if statement works fine. If I were writing this function from scratch, compartment would be a required column.
roninsightrx
left a comment
There was a problem hiding this comment.
looks good, but one last question about edge case



This PR changes nm_to_regimen so that it uses a map for compartment to dose type. This is useful for mipdeval, where this is needed.
This breaks backward compatibility because we now want an explicit declaration of the type of dosing instead of inferring from presence/absence of columns.
This function is used in two places: