fix: resolve interval parameter show raw PPTESTCDs instead of labels in NCA results #1305

[wangzhengdna-lang]

Issue

Closes #1305

Description

Interval parameters (e.g., AUCINT_0-24) displayed as raw PPTESTCDs in parameter selectors and boxplot y-axis labels instead of human-readable labels. The root cause: rename_interval_params() appends _start-end suffixes to PPTESTCD, but downstream label lookup against metadata_nca_parameters only has entries for base names (AUCINT), not the suffixed versions.

Fix

Reused existing parse_interval_parameter() to strip the range suffix before metadata lookup, then format the display as "{PPTEST} ({start}–{end})".

Two locations fixed:

1. selector_label() (inst/shiny/functions/selector_label.R) — Primary fix. The metadata_type == "parameter" branch now:

   • Parses each PPTESTCD via parse_interval_parameter() to extract base name
   • Joins against metadata_nca_parameters on the base name
   • Formats interval params as "AUC from T1 to T2 (0–24)"

2. .build_ylabel() (R/flexible_violinboxplot.R) — Secondary fix. Boxplot/violin plot y-axis labels now resolve interval parameter labels before appending the unit string.

Examples:

• AUCINT_0-24 → AUC from T1 to T2 (0–24) [hr*ng/mL]
• AUCINTda_0-24 → AUC from T1 to T2, dose-aware (0–24) [hr*ng/mL]

Definition of Done

• Interval parameters show readable labels in NCA Results parameter selectors
• Interval parameters show readable labels in Descriptive Statistics selectors
• Interval parameters show readable labels in Parameter Plots (boxplot) selectors
• Boxplot y-axis labels resolve interval parameter names
• Non-interval parameters unaffected (backward compatible)
• All unit tests pass (0 failures)

How to test

1. Load and launch: devtools::load_all(); aNCA::run_app()
2. Verify parameter selectors: In NCA > Results tab, choose a parameter for plots — interval parameters like AUCINT_0-24 should show as "AUC from T1 to T2 (0–24)" in the dropdown
3. Verify boxplot y-axis: In NCA > Parameter Plots, select an interval parameter — the y-axis label should show the resolved label with range, not raw AUCINT_0-24
4. Verify non-interval regression: Select a non-interval parameter (e.g. CMAX) — label should display normally as before

Contributor checklist

• Code passes lintr checks
• Code passes all unit tests — 0 failures, 68 warnings (all pre-existing), 3 skips
• New logic covered by unit tests — parse_interval_parameter() has existing tests in test-ratio_calculations.R
• App or package changes are reflected in NEWS
• Package version is incremented (0.1.0.9173 → 0.1.0.9174)
• R script works with the new implementation — N/A (no NCA logic changes)
• Settings upload works with the new implementation — N/A (no settings logic 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 parse_interval_parameter() from R/ratio_calculations.R — this function already correctly parses the PREFIX_start-end pattern and is tested in test-ratio_calculations.R. No new parsing logic introduced.
• The selector_label() test in test-selector_label.R uses a standalone simplified version of the function (not the Shiny module), so this change is not covered by that test file. Manual testing is required.
• Non-interval parameters are unaffected — the is_interval flag from parse_interval_parameter() gates the new formatting path.

[Shaakon35]

Almost good, the nice label : "AUC from T1 to T2 (0-24)" works for the violon plot and for the pickerinputs.

Still it does not work for the label shown in the datatables:

• NCA Results
• Descriptive Statistics

[Gero1999]

Actually @Shaakon35 I think is probably better to bet for "AUC from T1 to T2" so we prevent unnecessary hardcoding/bugs in the future. Otherwise the correct way would probably be "AUC from 0 to 24", "AUC from 24 to 48"...

[Shaakon35]

"AUC from 0 to 24", "AUC from 24 to 48"

Fine for me. Please refine the PR by:
changing from : "AUC from T1 to T2 (0-24)" to ""AUC from 0 to 24"

and fix the label shown in the datatables:

• NCA Results
• Descriptive Statistics

[wangzhengdna-lang]

Thanks for catching this. Two things fixed as your comments:

1. Label format changed per @Gero1999's suggestion: "AUC from T1 to T2 (0-24)" → "AUC from 0 to 24" — replaces T1/T2 placeholders with actual start/end values from the interval data.

2. NCA Results table headers — define_cols() in reactable.R now displays the resolved label as the column header (was showing raw AUCINT_0-12[Hours*ug/mL]). Tooltip also shows the same label.

3. Descriptive Statistics table headers — Added resolve_param_labels() in R/label_operators.R. It takes raw column names from the pivoted data frame (e.g. AUCINT_0-12[Hours*ug/mL]), strips the unit suffix, parses the PPTESTCD and range, then resolves the human-readable label from metadata_nca_parameters (e.g. "AUC from 0 to 12"). This label is set as the column's label attribute, which define_cols() uses for the displayed header. Called from reactable_server() so it covers both NCA Results and Descriptive Statistics.

[Shaakon35]

Thanks @wangzhengdna-lang

Almost there,

1. In descriptive statistics :

Could you please fix the pickerinput to have the label of AUCINT_0-12 or 0-24 as:

2. In the datatables NCA Results & Descriptive Statistics, it's almost correct, the name of the variable should be:

• "AUCINT_0-24(Hours*ug/mL)" - it's currently "AUC from 0 to 24"
  (The mouse hover (in black), is correct. it's: "AUC from 0 to 24")

[wangzhengdna-lang]

@Shaakon35 Thanks for the detailed feedback. Now all issues are fixed:

Picker in Descriptive Statistics — The virtualSelectInput now shows the raw PPTESTCD as the main label (e.g., AUCINT_0-12) with the resolved label as the subtitle/description (e.g., AUC from 0 to 12). Also fixed a gsub() vectorization bug in selector_label.R where case_when() + gsub() inside mutate() was using only the first start/end values for all interval parameters. Switched to the rowwise() pattern so each parameter gets its own range values.

Data table column headers — define_cols() in reactable.R now shows the raw column name with units as the header (e.g., AUCINT_0-24[Hours*ug/mL]), with the resolved label as the tooltip on hover (e.g., AUC from 0 to 24).

Fixed pre-existing line-length lints from translate_terms() calls in label_operators.R and pivot_wider_pknca_results.R.

[Shaakon35]

Issue 1: Duplicated add_label_attribute function

This function is now identical to the one in R/label_operators.R (line 202). Both copies received the same rowwise() + gsub T1/T2 replacement logic in this PR, which increases the maintenance burden — any future change needs to be applied in two places.

Proposed fix: Remove this copy entirely and call the exported version from label_operators.R. Since pivot_wider_pknca_results.R already calls it at line 167, just delete the local definition (lines 213–255) and rely on the exported one:

# In pivot_wider_pknca_results.R, line 167 already does:
pivoted_res <- add_label_attribute(pivoted_res, myres)
# This will resolve to the exported version in R/label_operators.R
# once the local duplicate is removed.

The local copy is marked @noRd @keywords internal, so removing it won't break any public API.

[Shaakon35]

Issue 2: rowwise() is slow — use vectorized ifelse() + gsub() instead

rowwise() evaluates each row independently, which is slow on large data frames. This same pattern appears in 3 locations in this PR (R/label_operators.R, R/pivot_wider_pknca_results.R, inst/shiny/functions/selector_label.R).

Since gsub() is already vectorized and start/end are column values, the whole rowwise() %>% mutate() %>% ungroup() block can be replaced with a single vectorized mutate():

Proposed fix (applies to all 3 locations):

# Replace lines 217-229 with:
mutate(
  PPTESTCD_cdisc = ifelse(
    type_interval == "manual",
    gsub("T2", as.character(end),
      gsub("T1", as.character(start), PPTESTCD_cdisc_raw)),
    PPTESTCD_cdisc
  )
) %>%

No rowwise() or ungroup() needed. Same result, fewer lines, better performance.

For selector_label.R, the equivalent would be:

mutate(
  desc = case_when(
    is_interval & !is.na(PPTEST) ~
      gsub("T2", as.character(end_dose),
        gsub("T1", as.character(start_dose), PPTEST)),
    !is.na(PPTEST) ~ PPTEST,
    TRUE ~ PPTESTCD
  )
) %>%

[Shaakon35]

@wangzhengdna-lang All good, it works for me :) Thanks for that!

Just address my two comments and fix the 4 failing checks:

• admiral CI/CD Workflows / Check / macos-latest (release) (pull_request)Failing after 3m
• admiral CI/CD Workflows / Check / ubuntu-latest (release) (pull_request)Failing after 4m
  etc

[Shaakon35]

@wangzhengdna-lang Can you take care of these two issues please?
#1321 (comment)
#1321 (comment)

[wangzhengdna-lang]

Thanks @Shaakon35 for the detailed review. Here's my analysis of the two issues:

Issue 1: Remove duplicate add_label_attribute — Accepted ✓

Good catch. Both copies are now identical after this PR, and maintaining two copies has been a documented pain point. Since pivot_wider_pknca_results.R:167 already calls add_label_attribute() by name, removing the local copy (lines 210–238) will automatically resolve to the exported version in label_operators.R. Will fix.

Issue 2: Replace rowwise() with vectorized ifelse() + gsub() — Cannot accept directly ✗

This is a known pitfall documented in the project's CLAUDE.md: gsub() inside mutate() only uses the first element of the replacement argument.

gsub("T1", as.character(start), PPTESTCD_cdisc_raw) — when start is a column vector like c(0, 12, 0, 24), gsub() only uses start[1] = 0 for ALL rows. So when myres$result contains multiple interval parameters with different start/end values (e.g., AUCINT_0-12 and AUCINT_0-24), the non-rowwise version produces:

Parameter	start	end	Expected	Without rowwise
AUCINT_0-12	0	12	"AUC from 0 to 12"	"AUC from 0 to 12" ✓
AUCINT_0-24	0	24	"AUC from 0 to 24"	"AUC from 0 to 12" ✗

AUCINT_0-24 incorrectly gets end[1] = 12.

Compromise: We can use group_by(start, end, type_interval) + vectorized gsub() inside a grouped mutate(). Since all rows in the same group share identical start/end, gsub() works correctly inside the group, and it's faster than rowwise():

group_by(start, end, type_interval) %>%
mutate(PPTESTCD_cdisc = if (type_interval[1] == "manual") {
  label <- PPTESTCD_cdisc_raw
  label <- gsub("T1", as.character(start[1]), label)
  label <- gsub("T2", as.character(end[1]), label)
  label
} else {
  PPTESTCD_cdisc
}) %>%
ungroup()

Would this approach work for you? If so, I'll apply it to all 3 locations.

[Shaakon35]

Compromise: We can use group_by(start, end, type_interval) + vectorized gsub() inside a grouped mutate(). Since all rows in the same group share identical start/end, gsub() works correctly inside the group, and it's faster than rowwise():

Sounds good! :)

[Shaakon35]

The new resolve_param_labels() function in R/label_operators.R has no unit tests. It contains non-trivial logic: unit-suffix stripping via regex, interval parsing, T1/T2 replacement, and a skip-if-already-labeled guard. This should have test coverage.

Suggested test cases for resolve_param_labels():

1. Interval column with unit suffix — e.g. column named AUCINT_0-12[hr*ng/mL] gets label "AUC from 0 to 12"
2. Interval column without unit suffix — e.g. AUCINT_0-24 gets label "AUC from 0 to 24"
3. Non-interval column — e.g. CMAX gets its standard PPTEST label from metadata
4. Column with existing label — label is preserved (not overwritten)
5. Unrecognized column — no label attribute set (returns NULL)

These can go in tests/testthat/test-label_operators.R alongside the existing add_label_attribute tests.

[wangzhengdna-lang]

@Shaakon35 Good catch — added 5 unit tests for resolve_param_labels() covering all suggested cases:

• Interval with unit suffix (AUCINT_0-12[hr*ng/mL] → "AUC from 0 to 12")
• Interval without unit suffix (AUCINT_0-24 → "AUC from 0 to 24")
• Non-interval column (CMAX → "Max Conc")
• Existing label preserved (not overwritten)
• Unrecognized column (no label set, returns NULL)

All 34 label_operators tests pass.

[wangzhengdna-lang]

@Shaakon35 Thanks for the detailed list

While writing them, 4 additional edge cases noticed may not covered by the current tests. Wanted to check if you'd like these included as well before pushing:

1. existing_label == col_name (apply_labels fallback) — apply_labels() sets attr(col, "label") <- col_name as a fallback for unrecognized columns. The current guard !identical(existing_label, col) would skip if label equals column name, meaning a column like CMAX with fallback label "CMAX" would never be resolved to "Max Conc". The test verifies this is correctly overwritten.

Dose-aware interval parameter (AUCINTda_0-24) — Verifies parse_interval_parameter() correctly identifies the base as AUCINTda (not AUCINT), and the metadata lookup returns "AUC from T1 to T2, dose-aware".

Multi-column mixed data frame — All current tests use single-column data frames. A 4-column test (interval + non-interval + unknown + existing-label) verifies the for-loop handles mixed column types in one pass without errors.

Empty data frame (0 rows) — The function iterates over names(data), not rows. Verifies it doesn't crash and still sets labels on column attributes.

[Shaakon35]

Yes, please include all 4. They cover real code paths:

1. existing_label == col_name — This is the actual production call chain in reactable.R (resolve_param_labels(apply_labels(data()))). Worth testing that the guard doesn't silently skip fallback labels.
2. AUCINTda_0-24 — The greedy regex behavior is non-obvious. Locks in correct parsing of the da suffix.
3. Multi-column mixed data frame — Catches potential cross-contamination between columns in the for-loop.
4. Empty data frame (0 rows) — One-liner, negligible cost, prevents future regressions on attr() over 0-length columns.

[Shaakon35]

@h5hoang Could you please have a review? :)

[wangzhengdna-lang]

@Shaakon35 Merge conflicts resolved with latest main:

• DESCRIPTION: version bumped to 0.1.0.9178 (main had moved to 0.1.0.9177)
• NEWS.md: #1305 entry already present on main, no conflict
• All 1851 tests pass (0 fail, 0 warn)

PR should now be mergeable.

[h5hoang]

Thanks @wangzhengdna-lang, functionality looks good! Just two nitpicks before the merge, both are items from your earlier discussion with @Shaakon35 that were agreed but don't seem to have made it into the pushed code on your branch:

1. add_label_attribute still has both definitions present (in label_operators.R and pivot_wider_pknca_results.R). I think you could drop the pivot_wider_pknca_results.R copy and keep the exported one in label_operators.R?

2. And for the rowwise() to group_by(start, end, type_interval) adoption, all three files still use rowwise() (in label_operators.R, pivot_wider_pknca_results.R, and selector_label.R). Output is correct either way, so this one's optional / could be a follow-up, your call

This is on me for reviewing so late, but some housekeeping updates need to be made like rebumping the DESCRIPTION version 0.1.0.9178 before merge.

Otherwise LGTM 🙏!

[wangzhengdna-lang]

@h5hoang All three addressed:

1. Duplicate add_label_attribute — Removed

Deleted the internal copy from pivot_wider_pknca_results.R. The exported version in label_operators.R is now the single source of truth. The call at pivot_wider_pknca_results.R:167 resolves to the exported one automatically.

2. rowwise() → group_by(start, end, type_interval) — Applied

Replaced in both label_operators.R and selector_label.R (the pivot_wider_pknca_results.R copy was removed). Since all rows within the same group share identical start/end values, gsub() inside grouped mutate() is correct and more efficient than rowwise().

3. Version — Already bumped

DESCRIPTION is at 0.1.0.9178 (rebased + bumped during the merge conflict resolution).

All 1851 tests pass.

[wangzhengdna-lang]

Please check if any other omissions need to be added or improved.

[Shaakon35]

@wangzheng Almost ready, could you just add the 4 units tests mentionned here:
#1321 (comment)

@Gero1999 How do we handle the merge with the fork of @wangzhengdna-lang

[Gero1999]

@Shaakon35 no problem, the procedure is exactly the same as any other repo branch