From 5dbfeffa7898ec336abdc39118cb9a7bcb217c28 Mon Sep 17 00:00:00 2001 From: Wang Zheng Date: Fri, 29 May 2026 19:58:29 +0800 Subject: [PATCH 1/4] fix: resolve transitive BLQ imputation dependencies for half.life chain (#1057) rm_impute_obs_params() previously used a single-level dependency check: parameters whose Depends did not directly reference an AUC param had their BLQ imputation removed. This missed the transitive chain half.life -> lambda.z -> aucinf.obs, causing standalone half.life to be calculated on raw data while AUCIFO internally used a different (imputed) half.life value. New .resolve_upstream_deps() helper recursively traces upstream dependencies (2 levels) from AUC parameters to identify all params in the calculation chain. half.life and lambda.z now correctly retain BLQ imputation when any AUC-dependent parameter is requested. Also adds the impute column guard from #1266 to prevent the "PKNCA_impute_method_FALSE" error when start_impute is FALSE. Co-Authored-By: Claude Opus 4.7 --- .gitignore | 3 ++- DESCRIPTION | 2 +- NEWS.md | 3 +++ R/intervals.R | 42 +++++++++++++++++++++++++++++++++++++----- 4 files changed, 43 insertions(+), 7 deletions(-) diff --git a/.gitignore b/.gitignore index ba2ec833b..eb776aafc 100644 --- a/.gitignore +++ b/.gitignore @@ -15,4 +15,5 @@ inst/shiny/log/* .DS_Store # {shinytest2}: Ignore new debug snapshots for `$expect_values()` *_.new.png -desktop.ini \ No newline at end of file +desktop.ini +CLAUDE.md \ No newline at end of file diff --git a/DESCRIPTION b/DESCRIPTION index 93220e9b3..3ff5bdc60 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -1,6 +1,6 @@ Package: aNCA Title: (Pre-)Clinical NCA in a Dynamic Shiny App -Version: 0.1.0.9175 +Version: 0.1.0.9176 Authors@R: c( person("Ercan", "Suekuer", email = "ercan.suekuer@roche.com", role = "aut", comment = c(ORCID = "0009-0001-1626-1526")), diff --git a/NEWS.md b/NEWS.md index 9ac7adfd1..a5aec6171 100644 --- a/NEWS.md +++ b/NEWS.md @@ -18,6 +18,9 @@ * Partial interval parameters section supports calculations beyond `AUCINT`: `RCAMINT`, `AUCINTD`, `CAVGINT`, and others. Table starts empty by default with a Remove Row button (#524, #1249) * "Min. Points for Half-life" setting added (range 2–10, default 3) (#1155) * BLQ imputation rules via `NCA Setup > Data Imputation` (#139) + +### Bug Fixes +* Half-life (LAMZHL) and lambda.z (LAMZ) now correctly retain BLQ imputation when half-life-dependent parameters (AUCIFO, AUCIFP) are requested. Previously `rm_impute_obs_params()` removed imputation from `half.life` because its dependency check was limited to one level — missing the transitive chain `half.life -> lambda.z -> aucinf.obs`. The dependency resolution now recursively traverses upstream dependencies to include all parameters in the AUC calculation chain (#1057). * General Exclusions section for in-app NCA exclusions, with "Excl. TLG" checkbox per entry (#851, #1018) * Parameter Exclusions tab: exclude individual PK parameter rows from descriptive statistics and ADPP export via PPSUMFL/PPSUMRSN flags (#1040) * NCA flag rules (NCAwXRS) from ADNCA standards — flagged records are excluded from NCA (#752) diff --git a/R/intervals.R b/R/intervals.R index 0db736fa0..9486459b2 100644 --- a/R/intervals.R +++ b/R/intervals.R @@ -295,6 +295,12 @@ update_main_intervals <- function( # and apply it only for non-observational parameters if (!is.null(blq_imputation_rule)) { + # Ensure impute column exists so dplyr mutate below references the + # column rather than the function parameter (which could be FALSE + # from YAML settings, causing "PKNCA_impute_method_FALSE" error). + if (!"impute" %in% names(data$intervals)) { + data$intervals$impute <- NA_character_ + } data$intervals <- data$intervals %>% mutate( impute = ifelse( @@ -324,16 +330,30 @@ update_main_intervals <- function( #' @import dplyr #' rm_impute_obs_params <- function(data, metadata_nca_parameters = metadata_nca_parameters) { - # Don't impute parameters that are not AUC dependent + # Don't impute parameters that are not AUC dependent. + # Start with directly AUC-related params, then resolve transitive + # dependencies (e.g. aucinf.obs -> lambda.z -> half.life) so that + # the half.life used within AUCINF reflects the same BLQ-imputed data + # as the AUC calculation itself (#1057). params_auc_dep <- metadata_nca_parameters %>% filter(grepl("auc|aumc", PKNCA) | grepl("auc", Depends)) %>% pull(PKNCA) + # Resolve transitive dependencies from AUC parameters. + # Two levels cover the full chain without reaching purely + # observational leaf parameters (cmax, tmax, tlast): + # Level 1: lambda.z, clast.obs (direct deps of aucinf.obs/pred) + # Level 2: half.life (dep of lambda.z, clast.pred) + needs_impute <- params_auc_dep + for (depth in 1:2) { + upstream <- .resolve_upstream_deps(metadata_nca_parameters, needs_impute) + upstream <- setdiff(upstream, needs_impute) + if (length(upstream) == 0) break + needs_impute <- c(needs_impute, upstream) + } + params_not_to_impute <- metadata_nca_parameters %>% - filter( - !grepl("auc|aumc", PKNCA), - !grepl(paste0(params_auc_dep, collapse = "|"), Depends) - ) %>% + filter(!PKNCA %in% needs_impute) %>% pull(PKNCA) %>% intersect(names(PKNCA::get.interval.cols())) @@ -367,3 +387,15 @@ rm_impute_obs_params <- function(data, metadata_nca_parameters = metadata_nca_pa data } + +#' Resolve the direct upstream dependencies of a set of PKNCA parameters. +#' Returns all parameter names listed in the Depends column for the given params. +#' @noRd +.resolve_upstream_deps <- function(metadata, params) { + dep_str <- metadata %>% + filter(PKNCA %in% params) %>% + pull(Depends) + dep_str <- dep_str[!is.na(dep_str) & dep_str != ""] + if (length(dep_str) == 0) return(character()) + unique(trimws(unlist(strsplit(dep_str, ",")))) +} From e1a9fa3db07cd8764de7bd7b0c0ed6c945f38960 Mon Sep 17 00:00:00 2001 From: Wang Zheng Date: Fri, 29 May 2026 20:19:59 +0800 Subject: [PATCH 2/4] refactor: use forward dependency walk in rm_impute_obs_params (#1057) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the reverse traversal (.resolve_upstream_deps) with a forward approach using a reverse dependency table (.build_rev_deps + .walk_forward_deps). Now follows the natural data-flow direction: for each parameter, checks whether any of its consumers (params that list it in Depends) are in the AUC chain. This makes the logic more intuitive — "half.life feeds lambda.z, which feeds aucinf.obs, therefore half.life keeps imputation." Extracted .walk_forward_deps() as a separate helper to reduce cyclomatic complexity. Co-Authored-By: Claude Opus 4.7 --- R/intervals.R | 66 ++++++++++++++++++++++++++++++++------------------- 1 file changed, 41 insertions(+), 25 deletions(-) diff --git a/R/intervals.R b/R/intervals.R index 9486459b2..f9e026113 100644 --- a/R/intervals.R +++ b/R/intervals.R @@ -331,26 +331,16 @@ update_main_intervals <- function( #' rm_impute_obs_params <- function(data, metadata_nca_parameters = metadata_nca_parameters) { # Don't impute parameters that are not AUC dependent. - # Start with directly AUC-related params, then resolve transitive - # dependencies (e.g. aucinf.obs -> lambda.z -> half.life) so that - # the half.life used within AUCINF reflects the same BLQ-imputed data - # as the AUC calculation itself (#1057). + # Parameters in the AUC calculation chain (half.life feeds lambda.z, + # which feeds aucinf.obs) must all use the same BLQ-imputed data (#1057). params_auc_dep <- metadata_nca_parameters %>% filter(grepl("auc|aumc", PKNCA) | grepl("auc", Depends)) %>% pull(PKNCA) - # Resolve transitive dependencies from AUC parameters. - # Two levels cover the full chain without reaching purely - # observational leaf parameters (cmax, tmax, tlast): - # Level 1: lambda.z, clast.obs (direct deps of aucinf.obs/pred) - # Level 2: half.life (dep of lambda.z, clast.pred) - needs_impute <- params_auc_dep - for (depth in 1:2) { - upstream <- .resolve_upstream_deps(metadata_nca_parameters, needs_impute) - upstream <- setdiff(upstream, needs_impute) - if (length(upstream) == 0) break - needs_impute <- c(needs_impute, upstream) - } + # Build reverse dependency table and walk forward along the data-flow + # direction to find all params whose consumers eventually reach an AUC param. + rev_deps <- .build_rev_deps(metadata_nca_parameters) + needs_impute <- .walk_forward_deps(params_auc_dep, rev_deps) params_not_to_impute <- metadata_nca_parameters %>% filter(!PKNCA %in% needs_impute) %>% @@ -388,14 +378,40 @@ rm_impute_obs_params <- function(data, metadata_nca_parameters = metadata_nca_pa data } -#' Resolve the direct upstream dependencies of a set of PKNCA parameters. -#' Returns all parameter names listed in the Depends column for the given params. +#' Build a reverse dependency table from the Depends column. +#' For each parameter A, returns which parameters list A as a dependency. +#' e.g., lambda.z lists half.life in Depends, so rev_deps$half.life includes lambda.z. #' @noRd -.resolve_upstream_deps <- function(metadata, params) { - dep_str <- metadata %>% - filter(PKNCA %in% params) %>% - pull(Depends) - dep_str <- dep_str[!is.na(dep_str) & dep_str != ""] - if (length(dep_str) == 0) return(character()) - unique(trimws(unlist(strsplit(dep_str, ",")))) +.build_rev_deps <- function(metadata) { + rev <- list() + for (i in seq_len(nrow(metadata))) { + pkg_name <- metadata$PKNCA[i] + dep_str <- metadata$Depends[i] + if (is.na(dep_str) || dep_str == "") next + dep_list <- trimws(strsplit(dep_str, ",")[[1]]) + for (d in dep_list) { + rev[[d]] <- unique(c(rev[[d]], pkg_name)) + } + } + rev +} + +#' Walk forward through the reverse dependency table from `start_set`. +#' At each iteration, finds params whose consumers include any param +#' already in the set — i.e., params that feed into the current chain. +#' Limited to 2 steps to avoid reaching purely observational leaf params. +#' @noRd +.walk_forward_deps <- function(start_set, rev_deps, max_depth = 2) { + needs <- start_set + for (depth in seq_len(max_depth)) { + newly_found <- character() + for (pkg in names(rev_deps)) { + if (!pkg %in% needs && any(rev_deps[[pkg]] %in% needs)) { + newly_found <- c(newly_found, pkg) + } + } + if (length(newly_found) == 0) break + needs <- c(needs, newly_found) + } + needs } From b99b58d40358ea67e9a4cf122f9ca7e66d84993b Mon Sep 17 00:00:00 2001 From: Wang Zheng Date: Thu, 4 Jun 2026 09:30:24 +0800 Subject: [PATCH 3/4] chore: remove CLAUDE.md from .gitignore; add tests for transitive dep helpers (#1057) - Remove CLAUDE.md from project .gitignore (per reviewer feedback) - Add 4 unit tests for .build_rev_deps() and .walk_forward_deps() covering reverse dep map construction, empty Depends handling, transitive chain resolution (half.life -> lambda.z -> aucinf.obs), and max_depth boundary enforcement Co-Authored-By: Claude Opus 4.7 --- .gitignore | 3 +- tests/testthat/test-intervals.R | 56 +++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 2 deletions(-) diff --git a/.gitignore b/.gitignore index eb776aafc..ba2ec833b 100644 --- a/.gitignore +++ b/.gitignore @@ -15,5 +15,4 @@ inst/shiny/log/* .DS_Store # {shinytest2}: Ignore new debug snapshots for `$expect_values()` *_.new.png -desktop.ini -CLAUDE.md \ No newline at end of file +desktop.ini \ No newline at end of file diff --git a/tests/testthat/test-intervals.R b/tests/testthat/test-intervals.R index 82a73b7e0..41eecc5d3 100644 --- a/tests/testthat/test-intervals.R +++ b/tests/testthat/test-intervals.R @@ -449,3 +449,59 @@ describe("rm_impute_obs_params", { expect_equal(result$intervals, intervals_before) }) }) + +describe(".build_rev_deps", { + it("builds reverse dependency map from Depends column", { + metadata <- data.frame( + PKNCA = c("half.life", "lambda.z", "aucinf.obs", "cmax"), + Depends = c("tmax, tlast", "half.life", "lambda.z", NA), + stringsAsFactors = FALSE + ) + result <- .build_rev_deps(metadata) + expect_type(result, "list") + expect_true("lambda.z" %in% result[["half.life"]]) + expect_true("aucinf.obs" %in% result[["lambda.z"]]) + expect_null(result[["cmax"]]) + }) + + it("handles empty Depends strings", { + metadata <- data.frame( + PKNCA = c("param1", "param2"), + Depends = c("", NA), + stringsAsFactors = FALSE + ) + result <- .build_rev_deps(metadata) + expect_type(result, "list") + }) +}) + +describe(".walk_forward_deps", { + it("finds half.life via transitive chain: half.life -> lambda.z -> aucinf.obs", { + metadata <- data.frame( + PKNCA = c("half.life", "lambda.z", "aucinf.obs", "cmax", "tmax"), + Depends = c("tmax, tlast", "half.life", "lambda.z", NA, NA), + stringsAsFactors = FALSE + ) + rev_deps <- .build_rev_deps(metadata) + start_set <- c("aucinf.obs") + result <- .walk_forward_deps(start_set, rev_deps) + expect_true("aucinf.obs" %in% result) + expect_true("lambda.z" %in% result) + expect_true("half.life" %in% result) + expect_false("cmax" %in% result) + expect_false("tmax" %in% result) + }) + + it("stops at max_depth=2 and does not reach leaf params at depth 3", { + metadata <- data.frame( + PKNCA = c("half.life", "lambda.z", "aucinf.obs"), + Depends = c("tmax", "half.life", "lambda.z"), + stringsAsFactors = FALSE + ) + rev_deps <- .build_rev_deps(metadata) + result <- .walk_forward_deps(c("aucinf.obs"), rev_deps) + expect_true("lambda.z" %in% result) + expect_true("half.life" %in% result) + expect_false("tmax" %in% result) + }) +}) From 737a8f3e828136448f30647a84b3a36671c48741 Mon Sep 17 00:00:00 2001 From: Wang Zheng Date: Thu, 4 Jun 2026 13:32:16 +0800 Subject: [PATCH 4/4] refactor: use explicit exclusion set instead of max_depth in walk_forward_deps (#1057) - Replace hardcoded max_depth=2 with explicit obs_params exclusion set (cmax, tmax, tlast). More robust - won't silently break if future parameters introduce deeper dependency chains. - Add rm_impute_obs_params integration test verifying half.life retains imputation when aucinf.obs is requested. - Ensure .gitignore has trailing newline. Co-Authored-By: Claude Opus 4.7 --- .gitignore | 2 +- R/intervals.R | 11 +++++++---- tests/testthat/test-intervals.R | 25 ++++++++++++++++++++++++- 3 files changed, 32 insertions(+), 6 deletions(-) diff --git a/.gitignore b/.gitignore index ba2ec833b..9e1aaeed3 100644 --- a/.gitignore +++ b/.gitignore @@ -15,4 +15,4 @@ inst/shiny/log/* .DS_Store # {shinytest2}: Ignore new debug snapshots for `$expect_values()` *_.new.png -desktop.ini \ No newline at end of file +desktop.ini diff --git a/R/intervals.R b/R/intervals.R index f9e026113..ed1e3b669 100644 --- a/R/intervals.R +++ b/R/intervals.R @@ -399,14 +399,17 @@ rm_impute_obs_params <- function(data, metadata_nca_parameters = metadata_nca_pa #' Walk forward through the reverse dependency table from `start_set`. #' At each iteration, finds params whose consumers include any param #' already in the set — i.e., params that feed into the current chain. -#' Limited to 2 steps to avoid reaching purely observational leaf params. +#' Stops when reaching purely observational leaf params (cmax, tmax, tlast) +#' to avoid including them in the imputation set. #' @noRd -.walk_forward_deps <- function(start_set, rev_deps, max_depth = 2) { +.walk_forward_deps <- function(start_set, rev_deps, + obs_params = c("cmax", "tmax", "tlast")) { needs <- start_set - for (depth in seq_len(max_depth)) { + repeat { newly_found <- character() for (pkg in names(rev_deps)) { - if (!pkg %in% needs && any(rev_deps[[pkg]] %in% needs)) { + if (!pkg %in% needs && !pkg %in% obs_params && + any(rev_deps[[pkg]] %in% needs)) { newly_found <- c(newly_found, pkg) } } diff --git a/tests/testthat/test-intervals.R b/tests/testthat/test-intervals.R index 41eecc5d3..9d0c0517f 100644 --- a/tests/testthat/test-intervals.R +++ b/tests/testthat/test-intervals.R @@ -492,7 +492,7 @@ describe(".walk_forward_deps", { expect_false("tmax" %in% result) }) - it("stops at max_depth=2 and does not reach leaf params at depth 3", { + it("excludes observational leaf params (cmax, tmax, tlast) regardless of depth", { metadata <- data.frame( PKNCA = c("half.life", "lambda.z", "aucinf.obs"), Depends = c("tmax", "half.life", "lambda.z"), @@ -502,6 +502,29 @@ describe(".walk_forward_deps", { result <- .walk_forward_deps(c("aucinf.obs"), rev_deps) expect_true("lambda.z" %in% result) expect_true("half.life" %in% result) + # tmax is in default obs_params → excluded regardless of depth expect_false("tmax" %in% result) }) }) + +describe("rm_impute_obs_params integration", { + it("retains imputation for half.life when aucinf.obs is requested (#1057)", { + data <- FIXTURE_PKNCA_DATA + # Mark all intervals as having imputation + if (!"impute" %in% names(data$intervals)) { + data$intervals$impute <- "blq" + } else { + data$intervals$impute[is.na(data$intervals$impute)] <- "blq" + } + # Request only aucinf.obs and half.life + for (col in names(data$intervals)) { + if (col %in% c("start", "end", "impute", "type_interval")) next + data$intervals[[col]] <- col %in% c("aucinf.obs", "half.life") + } + result <- rm_impute_obs_params(data, metadata_nca_parameters) + # half.life should NOT have imputation removed (it feeds aucinf.obs) + expect_false( + any(grepl("half.life", result$intervals$impute_not_for_params %||% character())) + ) + }) +})