-
Notifications
You must be signed in to change notification settings - Fork 15
Add compartments cmt to regimens in nm_to_regimen #128
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |



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: