Bug: derive_param_first_event() - AVALN

[kabis-ops]

What happened?

The function derive AVALN instead of AVAL

Session Information

R version 3.6.3 (2020-02-29)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Red Hat Enterprise Linux

Matrix products: default
BLAS/LAPACK: /usr/lib64/libopenblas-r0.2.20.so

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

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

other attached packages:
[1] lubridate_1.7.4 roxygen2_7.2.0 rlang_1.0.2 admiral.test_0.2.0 admiral_0.7.0
[6] dplyr_0.8.5 admiralonco_0.0.0 testthat_3.0.0

loaded via a namespace (and not attached):
[1] Rcpp_1.0.3 pillar_1.4.3 compiler_3.6.3 remotes_2.1.1 prettyunits_1.1.1
[6] tools_3.6.3 digest_0.6.25 pkgbuild_1.0.6 pkgload_1.0.2 memoise_1.1.0
[11] lifecycle_1.0.1 tibble_3.0.0 pkgconfig_2.0.3 cli_3.3.0 rstudioapi_0.11
[16] xfun_0.30 xml2_1.3.3 knitr_1.39 withr_2.4.3 stringr_1.4.0
[21] fs_1.5.2 desc_1.4.0 vctrs_0.3.8 devtools_2.2.2 hms_0.5.3
[26] rprojroot_1.3-2 tidyselect_1.0.0 glue_1.6.0 R6_2.4.1 processx_3.5.2
[31] sessioninfo_1.1.1 tidyr_1.0.2 purrr_0.3.3 callr_3.7.0 magrittr_2.0.3
[36] usethis_2.1.5 backports_1.1.5 ps_1.6.0 ellipsis_0.3.2 assertthat_0.2.1
[41] diffdf_1.0.4 renv_0.13.0 stringi_1.4.6 crayon_1.3.4

Reproducible Example

Example from the reference page

[kabis-ops]

@bundfussr , @bms63: FYI

[bms63]

Thanks for the bug report! Do you have some reproducible code that shows how this happens?

[bundfussr]

Good catch, @kabis-ops .

@bms63 , @thomas-neitmann , does this justify a bug fix release?

[thomas-neitmann]

@bms63 This is taken straight from our website. Here's the code:

library(dplyr)
library(lubridate)

# Derive a new parameter for the first disease progression (PD)
adsl <- tibble::tribble(
  ~USUBJID, ~DTHDT,
  "1",      ymd("2022-05-13"),
  "2",      ymd(""),
  "3",      ymd("")
) %>%
  mutate(STUDYID = "XX1234")

adrs <- tibble::tribble(
  ~USUBJID, ~ADTC,        ~AVALC,
  "1",      "2020-01-02", "PR",
  "1",      "2020-02-01", "CR",
  "1",      "2020-03-01", "CR",
  "1",      "2020-04-01", "SD",
  "2",      "2021-06-15", "SD",
  "2",      "2021-07-16", "PD",
  "2",      "2021-09-14", "PD"
) %>%
  mutate(
    STUDYID = "XX1234",
    ADT = ymd(ADTC),
    PARAMCD = "OVR",
    PARAM = "Overall Response",
    ANL01FL = "Y"
  ) %>%
  select(-ADTC)

derive_param_first_event(
  adrs,
  dataset_adsl = adsl,
  dataset_source = adrs,
  filter_source = PARAMCD == "OVR" & AVALC == "PD",
  date_var = ADT,
  set_values_to = vars(
    PARAMCD = "PD",
    PARAM = "Disease Progression",
    ANL01FL = "Y"
  )
)

[thomas-neitmann]

@bundfussr I would tend to say yes. What do you think?

[bms63]

Should we fix those other bugs as well if we are doing a bug fix release?

[thomas-neitmann]

I'd say so. Which are the other ones?

[bms63]

#1206 and #1207 ?

[bundfussr]

I think a bug fix release is a good idea. For the bug fixes we can not branch from devel. I would propose that we create a release branch branching from main. The bug fix branches branch from release and are merged to release and for the bug fix release we merge release into main and devel.

What do you think? Does this make sense?

[thomas-neitmann]

That makes a lot of sense @bundfussr! The only thing I'd suggest how about we call it hotfix rather than the more "generic" release?

[bundfussr]

I suggested release because we could also use it for normal releases. This would avoid that something is merged in while we are testing the release candidate.

[bms63]

What will be the version number of our hotfix/release? I guess since it it a patch we should do 0.7.1

https://semver.org/

[bundfussr]

Yes, 0.7.1 makes sense to me.

Should we create a "Release Strategy" vignette to write down our strategy for patch releases and normal releases? (Of course, the vignette would not be part of the patch but part of admiral 0.8.0.)

[bms63]

YEs we definitely need to write this down.

Can this be part of the template for CRAN releases?

I guess a vignette would be handy for other admiral extension project on how to handle this issue.

[bundfussr]

Should we add a hotfix label to the issues to be released in the bug fix release?

[thomas-neitmann]

@bundfussr Yes 👍

[kabis-ops]

Hi @bms63 , @thomas-neitmann @bundfussr

In addition to the AVALN issue,

We are planning to use derive_param_first_event in our admiralonco functions.
In admiralonco some conventions have been made regarding the adsl variables to populate when we create a new record/PARAMCD (see the discussion here):

• We keep all variables from the input dataset.
• For subjects without observations in the input dataset we keep all variables from ADSL which are also in the input dataset.
• Variables which should not be populated for the new parameter or populated differently (e.g., RSSTRESC, VISIT, PARCATy, ANLzzFL, ...) can be overwritten using the set_value_to parameter.

However the function derive_param_first_event does not support the bullet 2. It does not populate the adsl var present in the input datasets for the newly added records. See Reproducible example 😃 with AGE in ADRS.

adsl <- tibble::tribble(
  ~USUBJID, ~AGE,
  "1",78
) 
adrs <- tibble::tribble(
  ~USUBJID, ~ADTC, ~PARAMCD, ~AVALC,
  "1", "2020-01-02", "OVR", "PR",
  "1", "2020-02-01", "OVR", "CR",
  "1", "2020-03-01", "OVR", "PD",
  "1", "2020-04-01", "OVR", "SD"
)%>%
  mutate(  ADT = ymd(ADTC)) %>%
  select(-ADTC) %>%
  left_join(adsl, by="USUBJID")

derive_param_first_event(
  dataset = adrs,
  dataset_adsl = adsl,
  dataset_source = adrs,
  filter_source = PARAMCD == "OVR" & AVALC == "PD",
  date_var = ADT,
  subject_keys = vars( USUBJID),
  set_values_to = vars(PARAMCD = "PD"),
  check_type = "warning"
)

[rossfarrugia]

RE: "Release Strategy" vignette - consider whether it needs to be a vignette on admiral site (as most users may not need such detail) vs including it somewhere like https://github.com/pharmaverse/admiraltemplate/blob/main/README.md ?? just a thought!

[bms63]

RE: "Release Strategy" vignette - consider whether it needs to be a vignette on admiral site (as most users may not need such detail) vs including it somewhere like https://github.com/pharmaverse/admiraltemplate/blob/main/README.md ?? just a thought!

Is this in the right issue?

[rossfarrugia]

yeh sorry if it confused, was replying to Stefan's point around "Should we create a "Release Strategy" vignette....", but it is a tangential discussion that would warrant its own issue or thread on our slack.

[bundfussr]

@rossfarrugia , we have discussed it last Wednesday and decided to add it to the README (release schedule) and the git usage vignette (technical details). See #1220 and #1219.