Refactoring with checkmate and rlang

NEWS.md

@@ -11,6 +11,35 @@ the dosing including dose amount and route.
   parameters that use the full group (e.g. `aucint.all` and the other `aucint*`
   parameters) work when the input rows are not in time order (#568).
 
+* Migrated input validation across the package from base R checks
+  (`stopifnot()`, `is.character()`, `is.numeric()`, etc.) to `checkmate`
+  assertions, and standardized error/warning signaling on classed
+  `rlang::abort()`/`rlang::warn()` conditions (e.g. `pknca_error_*`,
+  `pknca_warning_*`) instead of unclassed base `stop()`/`warning()`. This
+  makes failure modes catchable by class rather than by matching message
+  text. As a side effect, some validations became stricter (rejecting
+  previously-accepted edge cases like non-finite numbers); see
+  the entries below for specifics.
+
+* `pk.calc.aucabove()` now requires `conc_above` to be finite. Previously,
+  `Inf` or `-Inf` were silently accepted and produced a degenerate result
+  (AUC of 0 for all profiles, since `conc - Inf` is always `-Inf`). Passing
+  a non-finite `conc_above` now raises an error instead.
+
+* `get_impute_method()` now requires `impute` to be an atomic scalar (via
+  `checkmate::assert_scalar()`). Previously, a bare `length(impute) == 1`
+  check meant a length-1 list (e.g. `list("start_conc0")`) could pass through,
+  relying on `%in%`'s implicit list coercion downstream instead of failing
+  clearly. Passing a list now raises an error instead.
+
+* `add.interval.col()` now validates the structure of `pptestcd_cdisc` and
+  `pptest_cdisc` rather than only checking that they are a character string or
+  a list.  A character value must be length 1 and non-missing, and a list must
+  have exactly one element named `route` whose value is a named list.  Values
+  that were previously accepted and would have produced incorrect CDISC output
+  (for example a multi-element character vector, or a list named something
+  other than `route`) now raise an error.
+
 * Business functions (used for calculations of means, etc.) now return NA_real_
   for empty inputs rather than giving an error (#559).
 
@@ -82,6 +111,16 @@ the dosing including dose amount and route.
 
 ## Breaking changes
 
+* The pre-existing `pknca_*` condition classes were renamed to carry an explicit
+  `error`/`warning`/`message` element, so code that catches them by class must be
+  updated.  For example, `pknca_conc_none` is now `pknca_warning_no_concentration`,
+  `pknca_no_intervals` is now `pknca_warning_no_intervals`, and
+  `pknca_all_warnings_no_results` is now `pknca_warning_no_results`.  One change
+  goes beyond renaming: the two classes `pknca_sparse_auclast_change_auclast` and
+  `pknca_sparse_aumclast_change_auc_type` were merged into the single class
+  `pknca_error_auc_last_type_override`, so they can no longer be caught
+  separately.
+
 * `pknca_units_table()` called on a `PKNCAdata` object now raises an error if
   unit columns within the same concentration group contain mixed values (e.g.,
   two different `concu` strings for the same subject group).  Previously, `NA`

R/001-add.interval.col.R

@@ -4,28 +4,84 @@ assign("options", NULL, envir=.PKNCAEnv)
 assign("summary", list(), envir=.PKNCAEnv)
 assign("interval.cols", list(), envir=.PKNCAEnv)
 
+# Validate a CDISC pptestcd/pptest argument: must be a character string, or a
+# named list with a "route" element containing a named list of
+# route-specific values (e.g. list(route = list(extravascular = ...))).
+# Not exported -- internal helper shared by add.interval.col().
+#' @param x The CDISC argument value to validate.
+#' @param arg_name The argument name used in error messages.
+#'
+#' @keywords internal
+#' @noRd
+validate_cdisc_arg <- function(x, arg_name) {
+  if (is.character(x)) {
+    if (!checkmate::test_string(x, na.ok = FALSE)) {
+      rlang::abort(
+        message = sprintf(
+          "`%s`, when a character string, must be length 1 and non-missing",
+          arg_name
+        ),
+        class = "pknca_error_cdisc_character_invalid"
+      )
+    }
+  } else if (is.list(x)) {
+    # `identical(names(x), "route")` also confirms that x has length 1
+    if (!identical(names(x), "route") ||
+        !is.list(x$route) ||
+        !checkmate::test_names(names(x$route), type = "named")) {
+      rlang::abort(
+        message = sprintf(
+          paste0(
+            "`%s`, when a list, must have exactly one named element, \"route\", ",
+            "whose value is itself a named list mapping route to value."
+          ),
+          arg_name
+        ),
+        class = "pknca_error_cdisc_route_mapping_invalid"
+      )
+    }
+  } else {
+    rlang::abort(
+      message = sprintf(
+        "`%s` must be a character string or a list",
+        arg_name
+      ),
+      class = "pknca_error_cdisc_invalid_type"
+    )
+  }
+}
+
 #' Add columns for calculations within PKNCA intervals
 #'
-#' @param name The column name as a character string
+#' @param name The column name as a non-empty character string (length 1,
+#'   may not be `NA` or `""`).
 #' @param FUN The function to run (as a character string) or `NA` if the
 #'   parameter is automatically calculated when calculating another parameter.
-#' @param values Valid values for the column
+#' @param values Valid values for the column: either a function used to
+#'   coerce/validate values (e.g. `as.numeric`) or a vector of allowed values
+#'   (e.g. `c(FALSE, TRUE)`).
+#' @param unit_type The type of units to use for assigning and converting
+#'   units. Must be one of the pre-defined unit types (see Details). This
+#'   argument is required and has no default; omitting it raises an error.
+#' @param pretty_name The name of the parameter to use for printing in summary
+#'   tables with units.  (If an analysis does not include units, then the normal
+#'   name is used.)
 #' @param depends Character vector of columns that must be run before this
 #'   column.
 #' @param desc A human-readable description of the parameter (<=40 characters to
 #'   comply with SDTM)
 #' @param sparse Is the calculation for sparse PK?
-#' @param unit_type The type of units to use for assigning and converting units.
-#' @param pretty_name The name of the parameter to use for printing in summary
-#'   tables with units.  (If an analysis does not include units, then the normal
-#'   name is used.)
 #' @param formalsmap A named list mapping parameter names in the function call
 #'   to NCA parameter names.  See the details for information on use of
 #'   `formalsmap`.
-#' @param datatype The type of data used for the calculation
+#' @param datatype The data type used for the calculation. The default is
+#'   `"interval"`, which is currently the only supported value. The
+#'   `"individual"` and `"population"` data types are reserved for future
+#'   use and will currently raise an error if selected.
 #' @param pptestcd_cdisc The CDISC PPTESTCD code for this parameter.  Can be a
 #'   character string for simple mappings, or a named list for route-dependent
-#'   mappings (e.g., `list(route = list(extravascular = "CLF/FO", intravascular
+#'   mappings with a `route` element whose value is itself a named list keyed
+#'   by route (e.g. `list(route = list(extravascular = "CLF/FO", intravascular
 #'   = "CLO"))`).  Defaults to `name` if not provided.
 #' @param pptest_cdisc The CDISC PPTEST name for this parameter.  Can be a
 #'   character string or a named list (same structure as `pptestcd_cdisc`).
@@ -96,27 +152,28 @@ add.interval.col <- function(name,
                              sparse=FALSE,
                              formalsmap=list(),
                              datatype=c("interval",
-                               "individual",
-                               "population"),
+                                        "individual",
+                                        "population"),
                              pptestcd_cdisc=NULL,
                              pptest_cdisc=NULL) {
   # Check inputs
-  if (!is.character(name)) {
-    stop("name must be a character string")
-  } else if (length(name) != 1) {
-    stop("name must have length == 1")
-  }
-  if (length(FUN) != 1) {
-    stop("FUN must have length == 1")
-  } else if (!(is.character(FUN) || is.na(FUN))) {
-    stop("FUN must be a character string or NA")
-  }
-  if (!is.null(depends)) {
-    if (!is.character(depends)) {
-      stop("'depends' must be NULL or a character vector")
-    }
+  checkmate::assert_character(x = name, len = 1, min.chars = 1, any.missing = FALSE)
+  checkmate::assert_character(x = FUN, len = 1, any.missing = TRUE) # allows NA
+  checkmate::assert_logical(x = sparse, len = 1, any.missing=FALSE)
+  checkmate::assert_character(x = pretty_name, len = 1, min.chars = 1, any.missing=FALSE)
+  checkmate::assert_character(x = desc, len = 1, any.missing=FALSE)
+  checkmate::assert_character(x = depends, null.ok = TRUE)  
+
+  # `values` must be either a function (used to validate/coerce) or a vector
+  # of allowed values -- both are acceptable, so just ensure it was supplied
+  # and is one of those two forms.
+  if (!is.function(values) && !is.vector(values)) {
+    rlang::abort(
+      message = "`values` must be a function or a vector of allowed values",
+      class = "pknca_error_values_invalid"
+    )
   }
-  checkmate::assert_logical(sparse, any.missing=FALSE, len=1)
+
   unit_type <-
     match.arg(
       unit_type,
@@ -132,42 +189,62 @@ add.interval.col <- function(name,
         "clearance", "renal_clearance", "renal_clearance_dosenorm"
       )
     )
-  stopifnot("pretty_name must be a scalar"=length(pretty_name) == 1)
-  stopifnot("pretty_name must be a character"=is.character(pretty_name))
-  stopifnot("pretty_name must not be an empty string"=nchar(pretty_name) > 0)
+
+  # Validate datatype (only "interval" is currently supported)
   datatype <- match.arg(datatype)
-  if (!(datatype %in% "interval")) {
-    stop("Only the 'interval' datatype is currently supported.")
-  }
-  if (length(desc) != 1) {
-    stop("desc must have length == 1")
-  } else if (!is.character(desc)) {
-    stop("desc must be a character string")
-  }
-  if (!is.list(formalsmap)) {
-    stop("formalsmap must be a list")
-  } else if (length(formalsmap) > 0 &&
-             is.null(names(formalsmap))) {
-    stop("formalsmap must be a named list")
-  } else if (length(formalsmap) > 0 &&
-             is.na(FUN)) {
-    stop("formalsmap may not be given when FUN is NA.")
-  } else if (!all(nchar(names(formalsmap)) > 0)) {
-    stop("All formalsmap elements must be named")
+  checkmate::assert_choice(x = datatype, choices = "interval")
+
+  # Validate formalsmap
+  checkmate::assert_list(
+    x = formalsmap,
+    names = if (length(formalsmap) > 0) "unique" else NULL
+  )
+
+  # Validate formalsmap and function compatibility
+  if (length(formalsmap) > 0) {
+    # Ensure FUN exists
+    if (is.na(FUN)) {
+      rlang::abort(
+        message = "`formalsmap` may not be provided when `FUN` is NA",
+        class = "pknca_error_formalsmap_with_na_fun"
+      )
+    }
+    # Ensure formalsmap names are unique
+    checkmate::assert_character(x = names(formalsmap), min.chars = 1, any.missing = FALSE)
   }
+
   # Ensure that the function exists
-  if (!is.na(FUN) &&
-      length(utils::getAnywhere(FUN)$objs) == 0) {
-    stop("The function named '", FUN, "' is not defined.  Please define the function before calling add.interval.col.")
-  }
-  if (!is.na(FUN) &&
-      length(formalsmap) > 0) {
-    # Ensure that the formalsmap parameters are all in the list of
-    # formal arguments to the function.
-    if (!all(names(formalsmap) %in% names(formals(utils::getAnywhere(FUN)$objs[[1]])))) {
-      stop("All names for the formalsmap list must be arguments to the function.")
+  if (!is.na(FUN)) {
+    # Ensure that the function exists
+    fun_obj <- utils::getAnywhere(FUN)
+    if (length(fun_obj$objs) == 0) {
+      rlang::abort(
+        message = sprintf(
+          "The function named '%s' is not defined. Please define it before calling add.interval.col().",
+          FUN
+        ),
+        class = "pknca_error_fun_not_found"
+      ) 
+    }
+
+    # Validate formalsmap parameters match function formals
+    if (length(formalsmap) > 0) {
+      fun_formals <- names(formals(fun_obj$objs[[1]]))
+      invalid_formals <- setdiff(names(formalsmap), fun_formals)
+      if (length(invalid_formals) > 0) {
+        rlang::abort(
+          message = sprintf(
+            "All names in `formalsmap` must be arguments to the function '%s'. Invalid names: %s",
+            FUN,
+            paste(dQuote(invalid_formals), collapse = ", ")
+          ),
+          class = "pknca_error_formalsmap_invalid_names"
+        )
+      }
     }
+
   }
+
   # Default CDISC mappings to name/desc when not provided
   if (is.null(pptestcd_cdisc)) {
     pptestcd_cdisc <- name
@@ -177,12 +254,9 @@ add.interval.col <- function(name,
   }
   # Validate CDISC arguments: must be a character string or a named list
   # with a "route" element containing named sub-elements
-  if (!is.character(pptestcd_cdisc) && !is.list(pptestcd_cdisc)) {
-    stop("pptestcd_cdisc must be a character string or a list")
-  }
-  if (!is.character(pptest_cdisc) && !is.list(pptest_cdisc)) {
-    stop("pptest_cdisc must be a character string or a list")
-  }
+  validate_cdisc_arg(pptestcd_cdisc, "pptestcd_cdisc")
+  validate_cdisc_arg(pptest_cdisc, "pptest_cdisc")
+
   current <- get("interval.cols", envir=.PKNCAEnv)
   current[[name]] <-
     list(
@@ -216,7 +290,7 @@ sort_interval_cols <- function() {
   myorder <- rep(NA, length(current))
   names(myorder) <- names(current)
   nextnum <- 1
-  while (any(is.na(myorder))) {
+  while (anyNA(myorder)) {
     for (nextorder in seq_along(myorder)[is.na(myorder)]) {
       if (length(current[[nextorder]]$depends) == 0) {
         # If it doesn't depend on anything then it can go next in order.
@@ -227,14 +301,20 @@ sort_interval_cols <- function() {
         deps <- unique(unlist(current[[nextorder]]$depends))
         missing_deps <- deps[!(deps %in% names(myorder))]
         if (length(missing_deps) > 0) {
-          stop(
-            "Invalid dependencies for interval column (please report this as a bug): ",
-            names(myorder)[nextorder],
-            " The following dependencies are missing: ",
-            paste(missing_deps, collapse=", ")
+          rlang::abort(
+            message = sprintf(
+              paste0(
+                "Invalid dependencies for interval column ",
+                "(please report this as a bug): %s ",
+                "The following dependencies are missing: %s"
+              ),
+              names(myorder)[nextorder],
+              paste(missing_deps, collapse = ", ")
+            ),
+            class = "pknca_error_invalid_dependency"
           )
         }
-        if (!any(is.na(myorder[deps]))) {
+        if (!anyNA(myorder[deps])) {
           myorder[nextorder] <- nextnum
           nextnum <- nextnum + 1
         }

R/002-pk.business.rules.R

@@ -53,7 +53,7 @@ pk.business <- function(FUN,
 geomean <- function(x, na.rm=FALSE) {
   if (na.rm)
     x <- stats::na.omit(x)
-  if (any(is.na(x))) {
+  if (anyNA(x)) {
     NA_real_
   } else if (any(x == 0)) {
     0

R/AIC.list.R

@@ -25,7 +25,12 @@ AIC.list <- function(object, ..., assess.best=TRUE) {
           if ("indentation" %in% names(ret)) {
             ret$indentation <- ret$indentation + 1
           } else {
-            stop("Unknown way to get a data.frame without indentation set.  This is likely a bug.") # nocov
+            # nocov start
+            rlang::abort(
+              message = "Unknown way to get a data.frame without indentation set. This is likely a bug.",
+              class = "pknca_error_internal_unknown_dataframe_indentation"
+            )
+            # nocov end
           }
         }
       }