Allow non-unique units columns (PKNCAconc & PKNCAdose) by Gero1999 · Pull Request #435 · humanpred/pknca

Gero1999 · 2025-06-10T07:48:44Z

Makes PKNCAdata$units table able to include any group variable stratifying the units in PKNCAconc / PKNCAdose .

Implementation Summary:

Added pknca_units_table_from_pknca. Creates a default units table based on a PKNCAconc object (and optionally PKNCAdose as well).
Introduced helper functions ensure_column_unit_exists and select_minimal_grouping_cols to support unit column validation and minimal grouping column selection.
Updated PKNCAdata.default to replace manual unit table construction with the new pknca_units_table_from_pknca function.
Added tests for pknca_units_table_from_pknca to verify its behavior with various scenarios, including:
Added new imports (dplyr, rlang, tidyr, utils) and exported pknca_units_table_from_pknca to make the new functionality accessible.

…ero1999/pknca into 416-custom_units_for_PKNCAdata

…PKNCAdata

Gero1999 · 2025-06-16T09:45:14Z

hey @billdenney I adapted some things we did on aNCA to include it as default behaviour in PKNCAdata. This should flexibilize the reading of unit columns in PKNCAconc (and PKNCAdose) related to #416. I added also tests to make sure this is translated into the results, but let me know if I miss something!

I also added some #TODO comments of the main changes I needed to do to adapt the function to PKNCA

billdenney · 2025-09-12T20:01:44Z

Instead of creating a new pknca_units_table_from_pknca() function, can you please make pknca_units_table() to be S3 generic with something like:

pknca_units_table <- function(concu, ...) {
  UseMethod("pknca_units_table")
}

pknca_units_table.default <- function(concu, doseu, amountu, timeu,
                              concu_pref = NULL, doseu_pref = NULL, amountu_pref = NULL, timeu_pref = NULL,
                              conversions = data.frame(), ...) {
  # the current function
}

pknca_units_table.PKNCAdata <- function(concu, ... conversions = data.frame()) {
  # Your function with conversions handled, too.
}

…ero1999/pknca into 416-custom_units_for_PKNCAdata

R/unit-support.R

billdenney · 2025-11-06T15:53:27Z

R/unit-support.R

+  all_unit_cols <- c(concu_col, amountu_col, timeu_col, doseu_col)
+
+  # Join dose units with concentration group columns and units
+  d_concu <- o_conc$data %>%


Please use base R for readability:

unit_cols <- c(group_conc_cols, concu_col, amountu_col, timeu_col) d_concu <- as.data.frame(o_conc) d_concu <- unique(d_concu[, intersect(names(d_concu), unit_cols)]) unit_cols <- c(group_conc_cols, doseu_col) d_doseu <- as.data.frame(o_dose) d_doseu <- unique(d_doseu[, intersect(names(d_doseu), unit_cols)])

billdenney · 2025-11-06T15:54:33Z

R/unit-support.R

+  groups_units_tbl <- merge(d_concu, d_doseu, all.x = TRUE) %>%
+    dplyr::mutate(dplyr::across(dplyr::everything(), ~ as.character(.))) %>%
+    dplyr::group_by(!!!rlang::syms(group_conc_cols)) %>%
+    tidyr::fill(!!!rlang::syms(all_unit_cols), .direction = "downup") %>%


Why is fill necessary here? It seems like it could go against the user's intent as they don't have units in their source data.

It is to ignore so if there are NA values assume they are the same as the units in the group as indicated in the test:

test_that("PKNCAdata units (#336)", { # Typical use d_conc <- data.frame(conc = 1, time = 0, concu_x = "A", timeu_x = "B", amountu_x = "C") d_dose <- data.frame(dose = 1, time = 0, doseu_x = "D") o_conc <- PKNCAconc(data = d_conc, conc~time, concu = "concu_x", timeu = "timeu_x") o_dose <- PKNCAdose(data = d_dose, dose~time, doseu = "doseu_x") o_data <- PKNCAdata(o_conc, o_dose) expect_equal( o_data$units, pknca_units_table(concu = "A", doseu = "D", timeu = "B") ) suppressWarnings(o_nca <- pk.nca(o_data)) expect_true("Cmax (A)" %in% names(summary(o_nca))) # NA unit values are ignored d_conc <- data.frame(conc = 1, time = 0:1, concu_x = c("A", NA), timeu_x = "B", amountu_x = "C") d_dose <- data.frame(dose = 1, time = 0, doseu_x = "D") o_conc <- PKNCAconc(data = d_conc, conc~time, concu = "concu_x", timeu = "timeu_x") o_dose <- PKNCAdose(data = d_dose, dose~time, doseu = "doseu_x") o_data <- PKNCAdata(o_conc, o_dose) expect_equal( o_data$units, pknca_units_table(concu = "A", doseu = "D", timeu = "B") )

But I think you are right, should be the user who fixes this when the situation occurs. I will remove this test and the fill action.

billdenney · 2025-11-06T15:55:19Z

R/unit-support.R

+
+  groups_units_tbl <- merge(d_concu, d_doseu, all.x = TRUE) %>%
+    dplyr::mutate(dplyr::across(dplyr::everything(), ~ as.character(.))) %>%
+    dplyr::group_by(!!!rlang::syms(group_conc_cols)) %>%


Please use dplyr::grouped_df() instead of indirection via rlang::syms().

billdenney · 2025-11-06T15:57:12Z

R/unit-support.R

+    unique()
+
+  groups_units_tbl <- merge(d_concu, d_doseu, all.x = TRUE) %>%
+    dplyr::mutate(dplyr::across(dplyr::everything(), ~ as.character(.))) %>%


Why do we need to convert everything to character? I think that it should all be character from the original input. If it is not (perhaps it's a factor), then we should likely handle factor conversion elsewhere. This seems like it is possibly hiding a data problem from the user (possibly causing a hard-to-trace bug in the future) and should be corrected in source data rather than here.

This is more to prevent NA values from creating conflicts during the group_by or the select_minimal_grouping_cols actions. It is only intended to ensure the units table is created, but it won't affect or change per ce the PKNCAdata input contents

billdenney · 2025-11-06T16:05:42Z

R/unit-support.R

+    stop(
+      "Units should be uniform at least across concentration groups. ",
+      "Review the units for the next group(s):\n",
+      paste(utils::capture.output(print(mismatching_units_groups)), collapse = "\n")


Rather than capture.output, please directly generate the desired string like:

do.call(paste, lapply(X = names(d), FUN = function(x) paste(x, d[[x]], sep = "=")))

billdenney · 2025-11-06T16:07:03Z

R/unit-support.R

+  units.are.all.na <- all(is.na(groups_units_tbl[,all_unit_cols]))
+  if (units.are.all.na) return(NULL)
+
+  groups_units_tbl %>%


Please convert this to a loop for readability (it will only likely be called a few times, so it should not be a performance issue).

billdenney · 2025-11-06T16:10:21Z

R/unit-support.R

+#' 2. If a column does not exist, it creates the column and assigns default values.
+#' 3. If not default values are provided, it assigns NA to the new column.
+#' @keywords Internal
+ensure_column_unit_exists <- function(pknca_obj, unit_name) {


I think that this is already handled as part of the pknca_set_units() function. Can this function be eliminated, and if not, can you please explain why not?

the problem I see is that pknca_set_units() does not guarantee the physical unit column to be made if not present. This helper/wrapper function does this by introducing NA values if not there

billdenney · 2025-11-06T16:11:42Z

R/unit-support.R

+}
+
+# Add globalVariables for NSE/dplyr/rlang/tidyr usage
+utils::globalVariables(c(


Please use double colon notation or rlang::.data rather than globalVariables()

billdenney · 2025-11-24T16:40:55Z

Please also merge the current main branch upon making the above changes.

…PKNCAdata (conflicts with no real issue in unit-support.R & NAMESPACE)

…PKNCAdata

…espacing instead f globalVariables

…make sure no additional warnings for row.names appear

…PKNCAdata

Gero1999 and others added 13 commits June 7, 2025 13:00

draft: default units_table for PKNCAdata

cc4d3c2

test: include tests for the default units table for PKNCAconc &PKNCAdose

eae9cfb

make fun to not need PKNCAdose

5fa7fd5

Merge branch 'humanpred:main' into 416-custom_units_for_PKNCAdata

5786c22

rename: fun to pknca_units_table_from_pknca

ebb0cab

docs: roxygenise funs

9ded111

Merge branch '416-custom_units_for_PKNCAdata' of https://github.com/G…

4b70ac2

…ero1999/pknca into 416-custom_units_for_PKNCAdata

fix: consider case when ret$o_dose = NA

518347f

fix: 1) columns order, 2) NULL if no units specified, 3) ignore NAs

41d8bb5

docs: roxygenise indicating additional dplyr funs

c4c6c69

Merge remote-tracking branch 'origin/main' into 416-custom_units_for_…

72e6d55

…PKNCAdata

fix: error for NA o_dose list & attr in units Xu_pref

64285e2

docs: fix example and update namespace

3b0aa18

Gero1999 marked this pull request as ready for review June 16, 2025 12:34

Gero1999 changed the title ~~Allow custom non-unique units (PPORRESU) based on PKNCAconc & PKNCAdose obj~~ Allow non-unique units columns (PKNCAconc & PKNCAdose) Jun 27, 2025

Gero1999 mentioned this pull request Sep 1, 2025

Collaboration: Things to consider for next PKNCA update pharmaverse/aNCA#481

Open

13 tasks

Gero1999 added 8 commits September 12, 2025 22:58

Merge branch '416-custom_units_for_PKNCAdata' of https://github.com/G…

bcfe407

…ero1999/pknca into 416-custom_units_for_PKNCAdata

feat: make pknca_units_table a method for PKNCAdata

02bff3d

test: adjust tests for PKNCAdata input

1c71730

test: refactor to follow PKNCA structure

04b393c

fix: typpo namespaces and docs

957091b

fix: issue for PKNCAdose not being present

3c7e6fc

fix: warning in test due to left_join

efb9118

fix check: update namespace & rm S3 generic message

1d1983d

billdenney requested changes Nov 6, 2025

View reviewed changes

suggestions: as_PKNCAconc, as_PKNCAdose

877a26e

Gero1999 marked this pull request as draft November 24, 2025 21:31

Gero1999 added 9 commits November 25, 2025 20:27

Merge remote-tracking branch 'origin/main' into 416-custom_units_for_…

67a5501

…PKNCAdata (conflicts with no real issue in unit-support.R & NAMESPACE)

Merge remote-tracking branch 'origin/main' into 416-custom_units_for_…

66b2fce

…PKNCAdata

add S3 methods for pknca_units_table to namespace

0c838aa

apply suggested changes: dplyr > base R, loop, rm capture.output, nam…

effdba4

…espacing instead f globalVariables

rm fill action, test assuming pknca_units_table ignoring NA values & …

e144557

…make sure no additional warnings for row.names appear

update NAMESPACE

bd6a468

Merge remote-tracking branch 'origin/main' into 416-custom_units_for_…

341c9ff

…PKNCAdata

change expect_error so ubuntu-latest (oldrel) also passes

b9ebb4d

reuse expect_error for the test

0507621

Conversation

Gero1999 commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Gero1999 commented Jun 16, 2025

Uh oh!

billdenney commented Sep 12, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

billdenney commented Nov 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Gero1999 commented Jun 10, 2025 •

edited

Loading