Skip to content

fix: include ATPTREF and ROUTE columns in ADPP export_cdisc output (#1276)#1339

Open
wangzhengdna-lang wants to merge 2 commits into
pharmaverse:mainfrom
wangzhengdna-lang:1276-fix/adpp-missing-atptref-route
Open

fix: include ATPTREF and ROUTE columns in ADPP export_cdisc output (#1276)#1339
wangzhengdna-lang wants to merge 2 commits into
pharmaverse:mainfrom
wangzhengdna-lang:1276-fix/adpp-missing-atptref-route

Conversation

@wangzhengdna-lang

@wangzhengdna-lang wangzhengdna-lang commented May 30, 2026

Copy link
Copy Markdown

Issue

Closes #1276

Description

export_cdisc() was not including ATPTREF and ROUTE columns in the ADPP output, even though these columns are essential for ratio calculations (accumulation ratios, bioavailability) and were already available in the source PKNCA data.

Root cause

export_cdisc() builds ADPP by selecting only columns listed in CDISC_COLS$ADPP$Variable. ATPTREF and ROUTE only had entries for ADNCA in metadata_nca_variables.csv — they were missing from ADPP, so they were silently dropped via any_of().

Fix

Added ATPTREF and ROUTE entries to the ADPP dataset metadata in metadata_nca_variables.csv, following the documented metadata_nca_variables update pattern.

Definition of Done

  • ATPTREF column appears in cdisc_dataset$adpp output
  • ROUTE column appears in cdisc_dataset$adpp output
  • ADNCA output unaffected
  • All existing tests pass

How to test

  1. Run NCA with any dataset
  2. Export CDISC datasets
  3. Verify adpp data frame contains ATPTREF and ROUTE columns

Files changed

File Change
data-raw/metadata_nca_variables.csv Added ATPTREF and ROUTE entries for ADPP dataset
data/metadata_nca_variables.rda Regenerated from CSV
R/data.R Updated row count in roxygen doc
man/metadata_nca_variables.Rd Auto-generated documentation
DESCRIPTION Version bumped
NEWS.md Bug fix entry added

Contributor checklist

  • Code passes lintr checks (0 lints)
  • Code passes all unit tests
  • New logic covered by unit tests — N/A (metadata addition, no new logic)
  • New logic is documented — N/A (metadata follows existing data-raw/ pattern)
  • App or package changes are reflected in NEWS
  • Package version is incremented
  • R script works with the new implementation — N/A
  • Settings upload works with the new implementation — N/A
  • 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

  • This follows the standard metadata_nca_variables update pattern documented in CLAUDE.md: edit CSV → regenerate .rda → update row count in R/data.R → run devtools::document()
  • The .rda file was regenerated using: metadata_nca_variables <- read.csv("data-raw/metadata_nca_variables.csv"); usethis::use_data(metadata_nca_variables, overwrite = TRUE)

Closes #1276

@Shaakon35

Copy link
Copy Markdown
Collaborator

No unit tests for ATPTREF/ROUTE in ADPP output

The PR adds ATPTREF and ROUTE to the ADPP metadata so they appear in export_cdisc() output, but there's no test verifying this.

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

it("includes ATPTREF and ROUTE in ADPP when present in source data", {
  modified <- test_pknca_res
  modified$data$conc$data$ATPTREF <- "Day 1"
  modified$data$conc$data$ROUTE <- "ORAL"
  result <- export_cdisc(modified)
  expect_true("ATPTREF" %in% names(result$adpp))
  expect_true("ROUTE" %in% names(result$adpp))
})

This ensures the metadata addition actually works end-to-end and prevents regressions.

@Shaakon35

Copy link
Copy Markdown
Collaborator

Incorrect row count in roxygen documentation

R/data.R and man/metadata_nca_variables.Rd both say "363 rows", but main already has 363 data rows. Adding 2 rows (ATPTREF, ROUTE) makes it 365.

Fix in R/data.R:

# Change:
#' @format A data frame with 363 rows and 5 variables:
# To:
#' @format A data frame with 365 rows and 5 variables:

Then run devtools::document() to regenerate the .Rd file (don't edit man/metadata_nca_variables.Rd manually).

@Shaakon35 Shaakon35 requested review from Gero1999 and Shaakon35 and removed request for Shaakon35 June 3, 2026 12:56

@wangzhengdna-lang wangzhengdna-lang left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Shaakon35 Both addressed:

1. Missing test — Added

Added a test in test-export_cdisc.R verifying ATPTREF and ROUTE appear in export_cdisc()$adpp output end-to-end. All 164 export tests pass.

2. Row count — Already correct

The roxygen doc says 363 rows, and nrow(metadata_nca_variables) on this branch is 363 (361 original + 2 added by this PR). The doc was already accurate after the CSV change — no update needed.

(The confusion may be due to wc -l counting lines differently: 365 lines = 1 header + 363 data rows + 1 trailing newline.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: ATPTREF and ROUTE missing from cdisc_dataset$adpp output

3 participants