Allow supplying TMDD dv_types argument to generate $ERROR code#15
Allow supplying TMDD dv_types argument to generate $ERROR code#15roninsightrx wants to merge 7 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for supplying a tmdd_dv_types argument to create_model() to control the DVID types used in TMDD model $ERROR code generation, and addresses an issue where IPRED may not be available in certain TMDD models by making IPRED and MDV optional variables in the default fit output table.
Changes:
- Added
tmdd_dv_typesparameter tocreate_model()with validation and type conversion logic - Modified
check_nm_table_variables()to optionally return invalid variables instead of throwing errors via newthrow_errorparameter - Updated
add_default_output_tables()to conditionally include IPRED and MDV in fit tables only when they're valid variables - Moved the
$TABLEsaddition code to execute after TMDD and metabolite processing to ensure proper variable availability checks
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| R/create_model.R | Added tmdd_dv_types parameter with validation logic; moved table generation after model structure modifications |
| R/add_table_to_model.R | Added throw_error parameter to check_nm_table_variables() to support non-error return of invalid variables |
| R/add_default_output_tables.R | Made IPRED and MDV optional in fit tables, only including them when they're valid model variables |
| man/check_nm_table_variables.Rd | Updated documentation for the new throw_error parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cli::cli_alert_info("Converting model into TMDD structure ({tmdd_type}).") | ||
| if(!is.null(tmdd_dv_types)) { | ||
| allowed_dv_types <- c("drug", "target", "complex", "drug_tot", "target_tot") | ||
| if(any(tmdd_dv_types %in% allowed_dv_types)) { |
There was a problem hiding this comment.
The validation logic has two bugs: 1) It checks the VALUES of tmdd_dv_types (integers like 1, 2) instead of the KEYS (names like "drug", "target"), and 2) The logic is inverted - it should error when keys are NOT in allowed_dv_types, not when they ARE. The correct condition should be: if(any(!names(tmdd_dv_types) %in% allowed_dv_types)). Currently, the check compares integers against a character vector which will always be FALSE, making the validation ineffective.
| if(any(tmdd_dv_types %in% allowed_dv_types)) { | |
| if(any(!names(tmdd_dv_types) %in% allowed_dv_types)) { |
| optional_vars <- c("MDV", "IPRED") | ||
| add_optional <- c() | ||
| for(variab in optional_vars) { | ||
| check_var <- check_nm_table_variables(model, variab, throw_error = FALSE) | ||
| if(is.null(check_var)) { # i.e. IPRED is declared as variable and we can safely add to table | ||
| add_optional <- c(add_optional, variab) | ||
| } | ||
| } |
There was a problem hiding this comment.
The new functionality for making IPRED and MDV optional in the fit table lacks test coverage. Consider adding test cases that verify: 1) When IPRED is not available (e.g., in some TMDD models), the fit table is created without it, and 2) When IPRED is available, it is included in the fit table. This is important since this change addresses a specific issue with TMDD models where IPRED may not be available.
| @@ -67,15 +71,18 @@ check_nm_table_variables <- function(model, variables) { | |||
| if (all(is_valid)) return() | |||
|
|
|||
| invalid_variables <- variables[!is_valid] | |||
| cli::cli_abort(c( | |||
| paste0( | |||
| "$TABLE variables must be any of the following: ", | |||
| "A data variable, a left-hand side variable in the model, ", | |||
| "or a reserved NONMEM $TABLE variable." | |||
| ), | |||
| "x" = "{.field {invalid_variables}} {?is/are} not {?a/} valid variable{?s}.", | |||
| "i" = "Did you make a typo?" | |||
| )) | |||
| if(throw_error) { | |||
| cli::cli_abort(c( | |||
| paste0( | |||
| "$TABLE variables must be any of the following: ", | |||
| "A data variable, a left-hand side variable in the model, ", | |||
| "or a reserved NONMEM $TABLE variable." | |||
| ), | |||
| "x" = "{.field {invalid_variables}} {?is/are} not {?a/} valid variable{?s}.", | |||
| "i" = "Did you make a typo?" | |||
| )) | |||
| } | |||
| invalid_variables | |||
There was a problem hiding this comment.
The new throw_error parameter and its behavior when set to FALSE lacks test coverage. Consider adding test cases that verify: 1) When throw_error = TRUE (default), invalid variables trigger an error as expected, and 2) When throw_error = FALSE, invalid variables are returned as a character vector without throwing an error. This will ensure the new dual-behavior functionality works correctly.
1 participant
No description provided.