Refactor of main MAP fit function #49
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mccarthy-m-g
approved these changes
Jun 20, 2025
mccarthy-m-g
left a comment
mccarthy-m-g
left a comment
There was a problem hiding this comment.
Nice improvements! Left some small suggestions below.
R/get_map_estimates.R
Outdated
Comment on lines
141
to
153
| ## Checks for estimation type | ||
| if(tolower(type) %in% c("map", "pls")) { | ||
| if(is.null(model) || is.null(data) || is.null(parameters) || is.null(omega) || is.null(regimen)) { | ||
| stop("The 'model', 'data', 'omega', 'regimen', and 'parameters' arguments are required.") | ||
| } | ||
| } | ||
| if(tolower(type) == "pls") { | ||
| weight_prior <- 0.001 | ||
| ## RK: Empirically determined to be a good penalty. | ||
| ## In principle, PLS is just MAP with very flat priors | ||
| } | ||
| if(tolower(type) %in% c("ls")) { | ||
| if(is.null(model) || is.null(data) || is.null(regimen)) { | ||
| stop("The 'model', 'data', and 'parameters' arguments are required.") | ||
| } | ||
| calc_ofv <- calc_ofv_ls | ||
| error <- list(prop = 0, add = 1) | ||
| } |
There was a problem hiding this comment.
You could refactor this into a function like:
check_estimation_type <- function(
type,
model,
data,
parameters,
omega,
regimen
) {
if(tolower(type) %in% c("map", "pls")) {
if(is.null(model) || is.null(data) || is.null(parameters) || is.null(omega) || is.null(regimen)) {
stop("The 'model', 'data', 'omega', 'regimen', and 'parameters' arguments are required.")
}
}
if(tolower(type) %in% c("ls")) {
if(is.null(model) || is.null(data) || is.null(regimen)) {
stop("The 'model', 'data', and 'parameters' arguments are required.")
}
}
type
}Then call this in the main function instead:
type <- check_estimation_type(type, model, data, parameters, omega, regimen)
if(tolower(type) %in% c("ls")) {
calc_ofv <- calc_ofv_ls
error <- list(prop = 0, add = 1)
}There are some other if statements in the main function that are similar (mainly just running a check and throwing an error if it's not met). It might be nice to refactor all of these into check_ functions too?
R/get_map_estimates.R
Outdated
Comment on lines
157
to
166
| if(!is.null(censoring) && !inherits(censoring, "character")) { | ||
| stop("Censoring argument requires label specifying column in dataset with censoring info.") | ||
| } | ||
| if(!("function" %in% class(model))) { | ||
| stop("The 'model' argument requires a function, e.g. a model defined using the new_ode_model() function from the PKPDsim package.") | ||
| } | ||
| if(!all(unlist(cols) %in% names(data))) { | ||
| stop("Expected column names were not found in data. Please use 'cols' argument to specify column names for independent and dependent variable.") | ||
| } | ||
| if("PKPDsim" %in% class(data)) { | ||
| if("comp" %in% names(data)) { | ||
| data <- data[data$comp == "obs",] | ||
| # data <- data[!duplicated(data$t),] | ||
| data$evid <- 0 | ||
| } | ||
| } | ||
| colnames(data) <- tolower(colnames(data)) | ||
| sig <- round(-log10(int_step_size)) | ||
| if("evid" %in% colnames(data)) { | ||
| data <- data[data$evid == 0,] | ||
| if(is.null(attr(model, "cpp")) || !attr(model, "cpp")) { | ||
| warning("Provided model is not PKPDsim model, using generic likelihood function.") | ||
| ll_func <- ll_func_generic | ||
| } |
There was a problem hiding this comment.
For example, these could all be check_ functions.
Comment on lines
+2
to
+6
| individual population eta relative | ||
| CL 6.699894 7.67 -0.671110 --o--|----- | ||
| V 87.000059 97.70 -3.668013 o-----|----- | ||
| Q 3.421152 3.00 1.313651 -----|--o-- | ||
| V2 50.000000 50.00 NA <NA> |
There was a problem hiding this comment.
I noticed we don't have a NEWS file for this package. It might be nice to add one so we can communicate/keep track of changes like this that affect output (i.e., "The print method for map_estimates() now uses sqrt(diag(PKPDsim::triangle_to_full(x$prior$omega$full))) instead of sqrt(diag(PKPDsim::triangle_to_full(x$prior$omega))) for etas").
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The function has become way too big and unwieldy. Splitting up in separate functions that can be unit-tested.
Also made code more readable in places, and added comments.
The new version is already much more readable, although there are still some places that could warrant refactor. But putting up for review now to get some signal on the current state.