Bug: derive_param_qtc() expects QT and RR in "msec"

[yurovska]

What happened?

In fact this is not really a bug but a sort of obsolescence. It was fine at the time when derive_param_qtc() was created. However, on 2022-06-24 the CDISC Submission Value for milliseconds was changed from msec to ms in the SDTM Controlled Terminology.

See line 1084 in SDTM Terminology Changes 2022-06-24.xls

So in all clinical trials started after that date, the following error is thrown:

Error in `derive_param_qtc()`:
! It is expected that "QT" has unit of "msec". In the input dataset the unit is "ms".
Run `rlang::last_trace()` to see where the error occurred.

Session Information

No response

Reproducible Example

library(tibble)

adeg <- tribble(
  ~USUBJID, ~PARAMCD, ~PARAM, ~AVAL, ~AVALU, ~VISIT,
  "01-701-1015", "HR", "Heart Rate (beats/min)", 70.14, "beats/min", "SCREENING",
  "01-701-1015", "HR", "Heart Rate (beats/min)", 62.66, "beats/min", "WEEK 1",
  "01-701-1015", "RR", "RR Duration (ms)",       710,   "ms",        "WEEK 2",
  "01-701-1015", "QT", "QT Duration (ms)",       370,   "ms",        "WEEK 2",
  "01-701-1028", "HR", "Heart Rate (beats/min)", 85.45, "beats/min", "SCREENING",
  "01-701-1028", "HR", "Heart Rate (beats/min)", 56.54, "beats/min", "WEEK 3",
  "01-701-1028", "RR", "RR Duration (ms)",       842,   "ms",        "WEEK 2",
  "01-701-1028", "QT", "QT Duration (ms)",       480,   "ms",        "WEEK 2",
  "01-701-1028", "QT", "QT Duration (ms)",       350,   "ms",        "WEEK 3"
)

derive_param_qtc(
  adeg,
  by_vars = exprs(USUBJID, VISIT),
  method = "Fridericia",
  set_values_to = exprs(
    PARAMCD = "QTCFR",
    PARAM = "QTcF - Fridericia's Correction Formula Rederived (ms)",
    AVALU = "ms"
  ),
  get_unit_expr = extract_unit(PARAM)
)

[yurovska]

The workaround found so far is to oversmart admiraldev::assert_unit() call by providing it with a dummy "msec" unit as literal via get_unit_expr:

derive_param_qtc(
  adeg,
  by_vars = exprs(USUBJID, VISIT),
  method = "Fridericia",
  set_values_to = exprs(
    PARAMCD = "QTCFR",
    PARAM = "QTcF - Fridericia's Correction Formula Rederived (ms)",
    AVALU = "ms"
  ),
  get_unit_expr = "msec"   # Workaround
)

However, you lose the check made by admiraldev::assert_unit() that the units are the same across all observations within each source parameter.

[bms63]

Could we allow the function to take in either ms or msec - @pharmaverse/admiral ?

[bms63]

Hi all, @pharmaverse/admiral We would like users to be able to use either msec or ms.

I believe the assert function can handle this, but it might take a little more thought in what that means if they use msec or ms? Let's discuss more!

[yurovska]

I looked through the code before posting the issue. Unfortunately, derive_param_qtc() checks units via admiraldev::assert_unit() that in turn checks the required_unit argument using admiraldev::assert_character_scalar(), i.e. it expects a single unit to compare with. Thus, admiraldev::assert_unit() would require an update and I have an idea how:

assert_unit <- function(dataset,
                        param,
                        required_unit,
                        get_unit_expr,
                        arg_name = rlang::caller_arg(required_unit),
                        message = NULL,
                        class = "assert_unit",
                        call = parent.frame()) {
  assert_data_frame(dataset, required_vars = exprs(PARAMCD))
  assert_character_scalar(param)
  
  #assert_character_scalar(required_unit)
  assert_character_vector(required_unit)    # <--- Updated here
  
  get_unit_expr <- enexpr(get_unit_expr)
  
  units <- dataset %>%
    mutate(`_unit` = !!get_unit_expr) %>%
    filter(PARAMCD == param & !is.na(`_unit`)) %>%
    pull(`_unit`) %>%
    unique()
  
  if (length(units) != 1L) {
    message <-
      message %||%
      "Multiple units {.val {units}} found for {.val {param}}. Please review and update the units."
    
    cli_abort(
      message = message,
      call = call,
      class = c(class, "assert-admiraldev")
    )
  }
  
  #if (tolower(units) != tolower(required_unit)) {
  if (tolower(units) %notin% tolower(required_unit)) {    # <--- Updated here
    message <-
      message %||%
      "It is expected that {.val {param}} has unit of {.or {required_unit}}.
       In the input dataset the unit is {.val {units}}."    # <--- Also changed .val to .or for required_unit
    
    cli_abort(
      message = message,
      call = call,
      class = c(class, "assert-admiraldev")
    )
  }
  
  invisible(dataset)
}

derive_param_qtc() would then be able to assert units as follows:

assert_unit(
  dataset,
  param = qt_code,
  required_unit = c("msec", "ms"),
  get_unit_expr = !!get_unit_expr
)
assert_unit(
  dataset,
  param = rr_code,
  required_unit = c("msec", "ms"),
  get_unit_expr = !!get_unit_expr
)

[bms63]

I looked through the code before posting the issue. Unfortunately, derive_param_qtc() checks units via admiraldev::assert_unit() that in turn checks the required_unit argument using admiraldev::assert_character_scalar(), i.e. it expects a single unit to compare with. Thus, admiraldev::assert_unit() would require an update and I have an idea how:

ah! that is unfortunate. I thought we had made assert_unit() a little more flexible. I guess the singular should of tipped me off. Maybe we need an assert_units() or assert_multiple_units() to allow some functions to have more flexibility now and for the future? @bundfussr @manciniedoardo WDYT?

Thanks again @yurovska for digging deep and giving code to look at. Helps move things along and really appreciate it!!

[bundfussr]

Maybe we need an assert_units() or assert_multiple_units() to allow some functions to have more flexibility now and for the future? @bundfussr @manciniedoardo WDYT?

I think we could just update the required_unit argument in assert_unit() (as suggested by @yurovska) without changing the function name. The singular makes sense because the function checks the unit of the specified parameter and units must not change within a parameter.

[bms63]

Sounds good! Do you mind making an issue in admiraldev for this update to assert_unit()

We should make sure to note in the documentation that it has this flexibility as well.

[bundfussr]

Sounds good! Do you mind making an issue in admiraldev for this update to assert_unit()

We should make sure to note in the documentation that it has this flexibility as well.

Done, see pharmaverse/admiraldev#468.