Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Closes #2185 additional implementation of keep_source_vars #2215

Closed
wants to merge 13 commits into from
Closed

Closes #2185 additional implementation of keep_source_vars #2215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
50e873d
feat: #2185 first implementation of `keep_source_vars`
jerryekohe Nov 1, 2023
cc05efe
Merge branch 'main' into 2185_make_keep_source_var_consistent
zdz2101 Nov 2, 2023
537936f
feat: #2185 add news blurb
jerryekohe Nov 2, 2023
f094d4d
test something
zdz2101 Nov 2, 2023
fd2f0b5
Merge branch 'main' into 2185_make_keep_source_var_consistent
zdz2101 Nov 3, 2023
163e407
Merge branch 'main' into 2185_make_keep_source_var_consistent
zdz2101 Nov 6, 2023
bec22b8
Merge branch 'main' into 2185_make_keep_source_var_consistent
zdz2101 Nov 7, 2023
4512455
address keep_source_vars edits/feedback
zdz2101 Nov 8, 2023
6f386c1
fix vignette as well
zdz2101 Nov 8, 2023
663fc2e
get past lintr
zdz2101 Nov 8, 2023
71fb435
chore: #2185 clean up errors
zdz2101 Nov 8, 2023
fe35445
get past styler
zdz2101 Nov 8, 2023
0533866
Merge branch 'main' into 2185_make_keep_source_var_consistent
zdz2101 Nov 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ were enhanced such that more than one summary variable can be derived, e.g.,
allow more control of the selection of records. It creates a temporary variable
for the event number, which can be used in `order`. (#2140)

- The `keep_source_vars` argument was added to `derive_param_tte()` for consistency (#2185)

## Breaking Changes

Expand Down
13 changes: 11 additions & 2 deletions R/derive_param_tte.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,14 @@
#'
#' A list of symbols created using `exprs()` is expected.
#'
#' @param keep_source_vars Variables to be kept in the new records
#'
#' A named list or tidyselect expressions created by `exprs()` defining the
#' variables to be kept for the new records. The variables specified for
#' `by_vars` and `set_values_to` need not be specified here as they are kept
#' automatically.
#'
#'
#' @details The following steps are performed to create the observations of the
#' new parameter:
#'
Expand Down Expand Up @@ -320,7 +328,8 @@ derive_param_tte <- function(dataset = NULL,
censor_conditions,
create_datetime = FALSE,
set_values_to,
subject_keys = get_admiral_option("subject_keys")) {
subject_keys = get_admiral_option("subject_keys"),
keep_source_vars = exprs(everything())) {
# checking and quoting #
assert_data_frame(dataset, optional = TRUE)
assert_vars(by_vars, optional = TRUE)
Expand Down Expand Up @@ -422,7 +431,7 @@ derive_param_tte <- function(dataset = NULL,
}

adsl <- dataset_adsl %>%
select(!!!adsl_vars)
select(!!!adsl_vars, !!!keep_source_vars)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Using keep_source_vars here is different from the other functions. Usually it is used to specify the variables which should be kept from the selected records. To be consistent we would need to add keep_source_vars to event_source() and censor_source(). However, I am not sure if this is a good idea (especially if the default is exprs(everything()) because different source datasets could be used. For example if event_source() uses ADRS and censor_source() uses ADSL, all ADRS and all ADSL variables would be added to the new records but the ADSL variables would be populated for censored records only.

Therefore I think we should not add keep_source_vars to derive_param_tte().

@bms63 , what do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bms63 @bundfussr I think it's fine if we don't implement it here too, derive_param_tte() seems hyperspecific for users, in which case we can just close the PR as well

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am fine to not implement this change. @zdz2101 when you return - can you make a note of this in the function re-work documents.


# create observations for new parameter #
new_param <- filter_extreme(
Expand Down
10 changes: 9 additions & 1 deletion man/derive_param_tte.Rd
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 7 additions & 5 deletions tests/testthat/test-derive_param_tte.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ test_that("derive_param_tte Test 1: new observations with analysis date are deri
PARAMCD = "OS",
PARAM = "Overall Survival"
) %>%
left_join(select(adsl, USUBJID, STARTDT = TRTSDT, STARTDTF = TRTSDTF), by = "USUBJID")
left_join(select(adsl, USUBJID, STARTDT = TRTSDT, STARTDTF = TRTSDTF, DTHFL, DTHDT, LSTALVDT), by = "USUBJID") # nolint

actual_output <- derive_param_tte(
dataset_adsl = adsl,
Expand Down Expand Up @@ -146,7 +146,7 @@ test_that("derive_param_tte Test 2: new parameter with analysis datetime is deri
PARAM = "Progression Free Survival"
) %>%
left_join(
select(adsl, USUBJID, STARTDTM = TRTSDTM, STARTDTF = TRTSDTF, STARTTMF = TRTSTMF),
select(adsl, USUBJID, STARTDTM = TRTSDTM, STARTDTF = TRTSDTF, STARTTMF = TRTSTMF, DTHFL, DTHDT), # nolint
by = "USUBJID"
)

Expand Down Expand Up @@ -251,7 +251,7 @@ test_that("derive_param_tte Test 3: no new observations for subjects not in ADSL
PARAM = "Duration of Response"
) %>%
left_join(
select(adsl, USUBJID, STARTDT = RSPDT),
select(adsl, USUBJID, STARTDT = RSPDT, DTHFL, DTHDT),
by = "USUBJID"
)

Expand Down Expand Up @@ -400,7 +400,8 @@ test_that("derive_param_tte Test 5: by_vars parameter works correctly", {
PARAM = paste("Time to First", AEDECOD, "Adverse Event"),
PARCAT1 = "TTAE",
PARCAT2 = AEDECOD
)
),
keep_source_vars = exprs(USUBJID)
),
expected_output,
keys = c("USUBJID", "PARAMCD")
Expand Down Expand Up @@ -796,7 +797,8 @@ test_that("derive_param_tte Test 11: ensuring ADT is not NA because of missing s
STUDYID = "AB42",
PARAMCD = "ANYAETTE",
PARAM = "Time to any first adverse event"
)
) %>%
left_join(select(adsl, USUBJID, LSTALVDT), by = "USUBJID")

expect_dfs_equal(
actual_output,
Expand Down
18 changes: 12 additions & 6 deletions vignettes/bds_tte.Rmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,8 @@ adtte <- derive_param_tte(
event_conditions = list(ae_ser_event),
censor_conditions = list(lastalive_censor),
source_datasets = list(adsl = adsl, adae = adae),
set_values_to = exprs(PARAMCD = "TTAESER", PARAM = "Time to First Serious AE")
set_values_to = exprs(PARAMCD = "TTAESER", PARAM = "Time to First Serious AE"),
keep_source_vars = exprs("USUBJID")
)
```
```{r, echo=FALSE}
Expand Down Expand Up @@ -184,7 +185,8 @@ adtte <- derive_param_tte(
start_date = TRTSDT,
event_conditions = list(death),
censor_conditions = list(lstalv),
set_values_to = exprs(PARAMCD = "OS", PARAM = "Overall Survival")
set_values_to = exprs(PARAMCD = "OS", PARAM = "Overall Survival"),
keep_source_vars = exprs("USUBJID")
)
```
```{r, echo=FALSE}
Expand Down Expand Up @@ -232,7 +234,8 @@ adtte <- derive_param_tte(
source_datasets = list(adsl = adsl),
event_conditions = list(death),
censor_conditions = list(lstalv),
set_values_to = exprs(PARAMCD = "OS", PARAM = "Overall Survival")
set_values_to = exprs(PARAMCD = "OS", PARAM = "Overall Survival"),
keep_source_vars = exprs("USUBJID")
)
```

Expand Down Expand Up @@ -399,7 +402,8 @@ adtte <- derive_param_tte(
start_date = TRTSDT,
event_conditions = list(pd, death),
censor_conditions = list(lastvisit, start),
set_values_to = exprs(PARAMCD = "PFS", PARAM = "Progression Free Survival")
set_values_to = exprs(PARAMCD = "PFS", PARAM = "Progression Free Survival"),
keep_source_vars = exprs(USUBJID)
)
```

Expand Down Expand Up @@ -497,7 +501,8 @@ adaette <- call_derivation(
adsl = adsl,
ae = filter(adae, TRTEMFL == "Y")
),
censor_conditions = list(observation_end)
censor_conditions = list(observation_end),
keep_source_vars = exprs("USUBJID")
)
```

Expand Down Expand Up @@ -594,7 +599,8 @@ adtte <- derive_param_tte(
PARAM = paste("Time to First", AEDECOD, "Adverse Event"),
PARCAT1 = "TTAE",
PARCAT2 = AEDECOD
)
),
keep_source_vars = exprs(USUBJID)
)
```

Expand Down
7 changes: 4 additions & 3 deletions vignettes/higher_order.Rmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,15 +168,16 @@ adaette <- call_derivation(
),
dataset_adsl = adsl,
source_datasets = list(adsl = adsl, adae = adae),
censor_conditions = list(lastalive_censor)
censor_conditions = list(lastalive_censor),
keep_source_vars = exprs(TRT01A, EOSSTT)
)
```

```{r, eval=TRUE, echo=FALSE}
adaette %>%
select(USUBJID, PARAMCD, STARTDT, ADT, CNSR, EVNTDESC, SRCDOM, SRCVAR) %>%
select(USUBJID, TRT01A, EOSSTT, PARAMCD, STARTDT, ADT, CNSR, EVNTDESC, SRCDOM, SRCVAR) %>%
arrange(USUBJID, PARAMCD) %>%
dataset_vignette(display_vars = exprs(USUBJID, PARAMCD, STARTDT, ADT, CNSR, EVNTDESC, SRCDOM, SRCVAR))
dataset_vignette(display_vars = exprs(USUBJID, TRT01A, EOSSTT, PARAMCD, STARTDT, ADT, CNSR, EVNTDESC, SRCDOM, SRCVAR))
```

Developing your ADaM scripts this way using `call_derivation()` could give the
Expand Down
Loading