Bug: `derive_param_wbc_abs()` doesn't handle environments properly

manciniedoardo commented 1 month ago

What happened?

A user at Roche called derive_param_wbc_abs() from within a wrapper function, and tried to pass to the former some objects defined only within the wrapper function. However, these objects were not recognised by derive_param_wbc_abs().

I think it's the same sort of error that we encountered and fixed for slice_derivation() and call_derivation() in #2244, it's symptomatic of poor environment handling by some of the {admiral} functions. We should probably do a more thorough review of the {admiral} codebase to identify other cases where this occurs, and proactively fix them.

Session Information

sessionInfo() R version 4.3.3 (2024-02-29) Platform: x86_64-pc-linux-gnu (64-bit) Running under: Ubuntu 22.04.4 LTS

Matrix products: default BLAS: /usr/lib/x86_64-linux-gnu/openblas-pthread/libblas.so.3 LAPACK: /usr/lib/x86_64-linux-gnu/openblas-pthread/libopenblasp-r0.3.20.so; LAPACK version 3.10.0

locale: [1] LC_CTYPE=en_US.UTF-8 LC_NUMERIC=C
[3] LC_TIME=en_US.UTF-8 LC_COLLATE=en_US.UTF-8
[5] LC_MONETARY=en_US.UTF-8 LC_MESSAGES=en_US.UTF-8
[7] LC_PAPER=en_US.UTF-8 LC_NAME=C
[9] LC_ADDRESS=C LC_TELEPHONE=C
[11] LC_MEASUREMENT=en_US.UTF-8 LC_IDENTIFICATION=C

time zone: Etc/UTC tzcode source: system (glibc)

attached base packages: [1] stats graphics grDevices utils datasets methods
[7] base

other attached packages: [1] admiral_1.1.1.9004 tibble_3.2.1

loaded via a namespace (and not attached): [1] tidyr_1.3.1 utf8_1.2.4 R6_2.5.1
[4] lubridate_1.9.3 tidyselect_1.2.1 magrittr_2.0.3
[7] glue_1.7.0 stringr_1.5.1 pkgconfig_2.0.3
[10] timechange_0.3.0 generics_0.1.3 dplyr_1.1.4
[13] lifecycle_1.0.4 cli_3.6.2 fansi_1.0.6
[16] vctrs_0.6.5 withr_3.0.0 compiler_4.3.3
[19] admiraldev_1.1.0.9001 purrr_1.0.2 rstudioapi_0.16.0
[22] tools_4.3.3 hms_1.1.3 pillar_1.9.0
[25] rlang_1.1.3 stringi_1.8.3

Reproducible Example

This should work but does not:

library(tibble)
library(admiral)

test_lb <- tribble(
  ~USUBJID, ~PARAMCD, ~AVAL, ~PARAM, ~VISIT,
  "P01", "WBC", 33, "Leukocyte Count (10^9/L)", "CYCLE 1 DAY 1",
  "P01", "WBC", 38, "Leukocyte Count (10^9/L)", "CYCLE 2 DAY 1",
  "P01", "LYMLE", 0.90, "Lymphocytes (fraction of 1)", "CYCLE 1 DAY 1",
  "P01", "LYMLE", 0.70, "Lymphocytes (fraction of 1)", "CYCLE 2 DAY 1"
)

my_fn <- function(data_in){

  std_ranges <- tibble(
    ANRLO_LYMPHLS = 1,
    ANRHI_LYMPHLS = 4.8
  )

  derive_param_wbc_abs(
    dataset = data_in,
    by_vars = exprs(USUBJID, VISIT),
    set_values_to = exprs(
      PARAMCD = "LYMPH",
      PARAM = "Lymphocytes Abs (10^9/L)",
      DTYPE = "CALCULATION",
      ANRLO = std_ranges$ANRLO_LYMPHLS,
      ANRHI = std_ranges$ANRHI_LYMPHLS,
    ),
    get_unit_expr = extract_unit(PARAM),
    wbc_code = "WBC",
    diff_code = "LYMLE",
    diff_type = "fraction"
  )
}

my_fn(test_lb)

But it does work if you unravel the dummy function my_fn().

jimrothstein commented 1 month ago

I got it work by adding library(dplyr) to top.

Conflicts with base::filter() on my setup.

manciniedoardo commented 1 month ago

library(tibble) library(admiral)

test_lb <- tribble( ~USUBJID, ~PARAMCD, ~AVAL, ~PARAM, ~VISIT, "P01", "WBC", 33, "Leukocyte Count (10^9/L)", "CYCLE 1 DAY 1", "P01", "WBC", 38, "Leukocyte Count (10^9/L)", "CYCLE 2 DAY 1", "P01", "LYMLE", 0.90, "Lymphocytes (fraction of 1)", "CYCLE 1 DAY 1", "P01", "LYMLE", 0.70, "Lymphocytes (fraction of 1)", "CYCLE 2 DAY 1" )

my_fn <- function(data_in){

std_ranges <- tibble( ANRLO_LYMPHLS = 1, ANRHI_LYMPHLS = 4.8 )

derive_param_wbc_abs( dataset = data_in, by_vars = exprs(USUBJID, VISIT), set_values_to = exprs( PARAMCD = "LYMPH", PARAM = "Lymphocytes Abs (10^9/L)", DTYPE = "CALCULATION", ANRLO = std_ranges$ANRLO_LYMPHLS, ANRHI = std_ranges$ANRHI_LYMPHLS, ), get_unit_expr = extract_unit(PARAM), wbc_code = "WBC", diff_code = "LYMLE", diff_type = "fraction" ) }

my_fn(test_lb)

How so? I wouldn't have expected it to work with that extra call and indeed for me it does not?

bundfussr commented 1 month ago

The code works if

      ANRLO = !!std_ranges$ANRLO_LYMPHLS,
      ANRHI = !!std_ranges$ANRHI_LYMPHLS,

is used.

I wouldn't consider this as a bug because in the expression specified for set_values_to only columns of the dataset should be used but not objects from any parent environment.

To improve the user's experience we could catch the "object not found" error and provide some hints like "Did you misspell the object name or miss unquoting (see https://pharmaverse.github.io/admiral/articles/concepts_conventions.html#exprs for guidance on quoting and unquoting)?"

manciniedoardo commented 1 month ago

The code works if
      ANRLO = !!std_ranges$ANRLO_LYMPHLS,
      ANRHI = !!std_ranges$ANRHI_LYMPHLS,
is used.

I wouldn't consider this as a bug because in the expression specified for set_values_to only columns of the dataset should be used but not objects from any parent environment.

To improve the user's experience we could catch the "object not found" error and provide some hints like "Did you misspell the object name or miss unquoting (see https://pharmaverse.github.io/admiral/articles/concepts_conventions.html#exprs for guidance on quoting and unquoting)?"

Yes, this sounds like a good idea thank @bundfussr - maybe could be discussed in the environments WG.

pharmaverse / admiral