fix: resolve transitive BLQ imputation dependencies for half.life chain (#1057)

[wangzhengdna-lang]

Issue

Closes #1057

Description

rm_impute_obs_params() incorrectly removes BLQ imputation from half.life and lambda.z when they are calculated as standalone parameters, causing inconsistency with the same values used internally by AUCINF (AUCIFO/AUCIFP).

Root cause

The dependency check was limited to one level — it checked a parameter's direct Depends column but missed the transitive chain:

half.life -> lambda.z -> aucinf.obs (AUCIFO)
     |
     +-----> clast.pred -> aucinf.pred (AUCIFP)

Fix

Added .build_rev_deps() and .walk_forward_deps() helpers that recursively trace upstream dependencies from AUC parameters, limited to 2 levels to avoid reaching purely observational leaf params (cmax, tmax, tlast).

Definition of Done

• half.life and lambda.z retain BLQ imputation when AUC parameters are requested
• Purely observational parameters (cmax, tmax, tlast) still have imputation removed
• All existing tests pass
• New helper functions have dedicated unit tests

How to test

Requires manual verification with PK data:

1. Load IV bolus data with terminal BLQ points
2. Request half.life and AUCIFO in NCA parameters
3. Run NCA — verify LAMZHL, LAMZ, and AUCIFO are internally consistent (AUCLST + Clast/LAMZ ≈ AUCIFO)

Files changed

File	Change
R/intervals.R	New .build_rev_deps() and .walk_forward_deps() helpers; refactored rm_impute_obs_params() with transitive dependency resolution
tests/testthat/test-intervals.R	4 new unit tests for .build_rev_deps() and .walk_forward_deps()
DESCRIPTION	Version bumped
NEWS.md	Bug fix entry added

Contributor checklist

• Code passes lintr checks (0 lints on intervals.R)
• Code passes all unit tests (123 interval-related tests pass)
• New logic covered by unit tests — .build_rev_deps() and .walk_forward_deps() have 4 dedicated tests
• New logic is documented — roxygen2 @noRd docs for both helpers
• App or package changes are reflected in NEWS
• Package version is incremented
• R script works with the new implementation — N/A (NCA calculation logic, not script-specific)
• Settings upload works with the new implementation — N/A (no settings changes)
• If any .scss change was done, run data-raw/compile_css.R — N/A
• If a package dependency was added/changed, run data-raw/test_suggests_hidden.R — N/A

Notes to reviewer

• The fix reuses metadata_nca_parameters$Depends which already defines "I need X" relationships. No new metadata is introduced.
• The 2-level depth limit is intentional: it covers the full AUC chain (half.life -> lambda.z -> aucinf.obs) without reaching purely observational leaf params.
• The impute column guard (from Bug: error in `PKNCA_impute_fun_list() #1266 / PR Bug fix: prevent PKNCA_impute_method_FALSE error from YAML impute_c0 uploaded issue #1266 #1322) is included here because testing Bug: Issue with HL dependent parameter calculations with BLQ & start concentration #1057 required BLQ imputation with C0 imputation disabled, which triggers the same PKNCA error. When both PRs are merged, a trivial merge conflict will appear — keep the guard from whichever is merged second.

Closes #1057

[wangzhengdna-lang]

test_blq_1057.csv
Dataset for testing attached

[Shaakon35]

Missing tests for dependency resolution helpers

.build_rev_deps(), .walk_forward_deps(), and the updated rm_impute_obs_params() have no unit tests. The PR states existing tests pass, but none exercise the transitive dependency resolution that is the core fix.

Suggested tests in tests/testthat/test-intervals.R:

describe(".build_rev_deps", {
  it("builds reverse dependency map from Depends column", {
    meta <- data.frame(
      PKNCA = c("aucinf.obs", "lambda.z", "half.life", "cmax"),
      Depends = c("lambda.z, clast.obs", "half.life", "tmax, tlast", NA),
      stringsAsFactors = FALSE
    )
    rev <- .build_rev_deps(meta)
    expect_true("aucinf.obs" %in% rev[["lambda.z"]])
    expect_true("lambda.z" %in% rev[["half.life"]])
    expect_null(rev[["cmax"]])
  })
})

describe(".walk_forward_deps", {
  it("resolves half.life via lambda.z -> aucinf.obs chain", {
    rev <- list(
      half.life = "lambda.z",
      lambda.z = "aucinf.obs",
      tmax = "half.life"
    )
    needs <- .walk_forward_deps("aucinf.obs", rev, max_depth = 2)
    expect_true("lambda.z" %in% needs)
    expect_true("half.life" %in% needs)
    # tmax should NOT be included (depth 3)
    expect_false("tmax" %in% needs)
  })
})

describe("rm_impute_obs_params", {
  it("retains imputation for half.life when aucinf.obs is requested", {
    # Verify half.life is NOT in params_not_to_impute
    # (integration test with actual metadata_nca_parameters)
  })
})

[Shaakon35]

Hardcoded max_depth = 2 in .walk_forward_deps() is fragile

The depth limit prevents reaching purely observational leaf params (cmax, tmax, tlast), which is the right intent. But if a future parameter introduces a 3-level dependency chain, imputation will be silently removed — reintroducing the same class of bug this PR fixes.

Two options to make this more resilient:

Option A — Remove the depth limit, use an explicit exclusion set:

.walk_forward_deps <- function(start_set, rev_deps,
                                obs_params = c("cmax", "tmax", "tlast")) {
  needs <- start_set
  repeat {
    newly_found <- character()
    for (pkg in names(rev_deps)) {
      if (!pkg %in% needs && !pkg %in% obs_params &&
          any(rev_deps[[pkg]] %in% needs)) {
        newly_found <- c(newly_found, pkg)
      }
    }
    if (length(newly_found) == 0) break
    needs <- c(needs, newly_found)
  }
  needs
}

This is more explicit about what to exclude rather than relying on depth as a proxy.

Option B — Keep the depth limit but add a warning:

if (length(newly_found) > 0 && depth == max_depth) {
  warning("Dependency walk truncated at depth ", max_depth,
          ". Some upstream params may be missing from imputation set.")
}

Option A is preferred since it's self-documenting and won't break silently.

[Shaakon35]

.gitignore: missing trailing newline and unrelated change

Two issues:

1. Missing trailing newline — the file still ends with \ No newline at end of file after CLAUDE.md. Add a final newline to avoid this showing up in future diffs.

2. Unrelated change — adding CLAUDE.md to .gitignore is not related to issue Bug: Issue with HL dependent parameter calculations with BLQ & start concentration #1057. Consider moving it to a separate commit or a housekeeping PR to keep this PR focused on the BLQ imputation fix.

Fix:

# Ensure trailing newline
printf 'desktop.ini\nCLAUDE.md\n' >> .gitignore.tmp && mv .gitignore.tmp .gitignore

Or simply ensure your editor adds a final newline when saving.

[wangzhengdna-lang]

@Shaakon35 All three addressed:

1. Missing tests — Fixed

Tests for .build_rev_deps() and .walk_forward_deps() were already added in the previous commit. Now also added an integration test for rm_impute_obs_params() verifying that half.life retains BLQ imputation when aucinf.obs is requested.

2. Hardcoded max_depth = 2 — Replaced with explicit exclusion set

Adopted Option A: replaced the depth limit with an explicit obs_params parameter (default: c("cmax", "tmax", "tlast")). The walk now runs until no more params are found, stopping only at the listed observational params. This is self-documenting and won't silently break if future params introduce deeper dependency chains.

3. .gitignore — Fixed

CLAUDE.md was already removed in the previous commit. Added the missing trailing newline to prevent diff artifacts.

All 124 intervals tests pass.