Closes #73: add ADVS example and update ADSL example and corresponding specs #83
Conversation
…se build_from_derived() function
There was a problem hiding this comment.
I am so excited about this @Fanny-Gautier !!!! I will try to get to early next week.
@pharmaverse/admiral we got some new integrated examples coming online as a result of @Fanny-Gautier hard work. Anyone got time to help out with the review?
This aligns ADSL closer to admiral ADSL and introduces ADVS as another example
adam/adsl.qmd
Outdated
| where_sep_sheet = FALSE, | ||
| quiet = TRUE |
There was a problem hiding this comment.
Could we mention why we are setting these arguments like this?
There was a problem hiding this comment.
where_sep_sheet explained
quiet dropped and thus left to the default value since we do not have any warnings to mute
| ds_list = list("dm" = dm), | ||
| predecessor_only = FALSE, keep = TRUE) | ||
| ds_list = list("dm" = dm_suppdm, "suppdm" = dm_suppdm), | ||
| predecessor_only = FALSE, keep = FALSE) | ||
| head(adsl_preds, n=10) |
There was a problem hiding this comment.
could we display this in a prettier way using reactable package? https://github.com/pharmaverse/admiraldiscovery/blob/v0.3.0/R/interactive.R here is what Daniel uses it in admiraldiscovery
There was a problem hiding this comment.
It's not looking that great without customizing it:
Could you please suggest where I could save the customization that it's not directly in the code itself ?
There was a problem hiding this comment.
to compare with head()
There was a problem hiding this comment.
we could just create another folder called functions at the top-level. and store the fancy code in there. and then your code could call the reactable code from the functions folder
There was a problem hiding this comment.
but yeah agree the vanilla reactable looks sad
| Now we have the base dataset, we can start to create some variables. | ||
| There are a few options to create grouping variables and their corresponding numeric variables. | ||
|
|
||
| Option 1: We can start with creating the subgroups using the controlled terminology, in this case `AGEGR1`. |
There was a problem hiding this comment.
Could we make use of how the logs section does this? This is a pretty long section
There was a problem hiding this comment.
At least "Option 1" should be bold.
| Now we have sorted out what we can easily do with controlled terminology it is time to start deriving some variables. Here you could refer directly to using the `{admiral}` template and [vignette](https://pharmaverse.github.io/admiral/cran-release/articles/adsl.html) in practice, but for the purpose of this end-to-end ADaM vignette we will share a few exposure derivations from there. We derive the start and end of treatment (which requires dates to first be converted from DTC to DTM), the treatment duration, and the safety population flag. | ||
| ### Exposure derivations | ||
|
|
||
| Now we have sorted out what we can easily do with controlled terminology it is time to start deriving some variables. |
adam/adsl.qmd
Outdated
| derive_var_merged_exist_flag( | ||
| dataset_add = ex, | ||
| by_vars = exprs(STUDYID, USUBJID), | ||
| new_var = SAFFL, | ||
| condition = (EXDOSE > 0 | (EXDOSE == 0 & str_detect(EXTRT, "PLACEBO"))) | ||
| ) %>% | ||
| drop_unspec_vars(metacore) # This will drop any columns that aren't specified in the metacore object | ||
| drop_unspec_vars(metacore) # This will drop any columns that are not specified in the metacore object |
There was a problem hiding this comment.
ummm...should this step just go at the end or am i missing something?
There was a problem hiding this comment.
Got a bit reviewed. Looking so good.
Maybe we could use the link package like how we do in the blog to help make functions and packages linkable? https://github.com/pharmaverse/blog/blob/main/posts/2025-01-17_1.2_admiral.../1.2_admiral_release.qmd
@rossfarrugia and @pawelru this is nice package we use on blogging site that automatically creates links for functions and packages. very handy - maybe should become standard (add to checklist?)
Co-authored-by: Ben Straub <[email protected]>
|
As someone with no
Line 17 in 8fe8b6c
|
I have no strong opinion honestly speaking. In general, I'm not a fan of extending dependencies and keep things small and simple but if this helps and it's robust enough - let's go with it! |
Same here @bms63 - happy if you want to go ahead and make an issue for this one to get it added across all the blogs, if anyone has time to take it on. |
There was a problem hiding this comment.
Is it possible to add a TOC to the pages like in the admiral vignettes?
Do we need to repeat so much information from the admiral vignettes?
It's not clear to me what we want to show. Do we want to show how to create an ADSL or ADVS dataset or how admiral and other pharmaverse packages work together?
In the former case I wonder why we show this in two places (admiral vignettes and pharmaverse examples). In the later case I would reduce the number of admiral function calls and focus on those where other packages are used like checks or writing xpt files or those where other packages provide alternative functions like deriving grouping variables.
|
|
||
| head(adsl_ct, n=10) | ||
| ``` | ||
|
|
There was a problem hiding this comment.
I would swap option 1 and option 2 because using derive_vars_cat() is more general and more reviewer friendly while create_cat_var() works only in some scenarios.
| Now we have the base dataset, we can start to create some variables. | ||
| There are a few options to create grouping variables and their corresponding numeric variables. | ||
|
|
||
| Option 1: We can start with creating the subgroups using the controlled terminology, in this case `AGEGR1`. |
There was a problem hiding this comment.
At least "Option 1" should be bold.
| grp_var = AGEGR1, num_grp_var = AGEGR1N) %>% | ||
| grp_var = AGEGR1, num_grp_var = AGEGR1N) | ||
|
|
||
| head(adsl_ct, n=10) |
There was a problem hiding this comment.
The new variables should be displayed in the output.
| mutate(ARM = if_else(ARM == "Screen Failure", NA_character_, ARM), | ||
| TRT01P = if_else(TRT01P == "Screen Failure", NA_character_, TRT01P) | ||
| ) | ||
| out_var = RACEN) | ||
|
|
||
| head(adsl_ct, n=10) |
There was a problem hiding this comment.
The new variable should be displayed in the output.
| head(adsl_raw %>% select(STUDYID, USUBJID, TRTSDTM, TRTSTM, TRTSTMF, TRTSDT, TRTEDTM, TRTETMF, TRTEDT, TRTDURD, SAFFL), n=10) | ||
| ``` | ||
|
|
||
| This call returns the original data frame with the column `TRTSDTM`, `TRTSTMF`, `TRTEDTM`, and `TRTETMF` added. |
There was a problem hiding this comment.
I think this should be moved or rephrased because there is more than one call in the chunk above and more than the four columns are created.
| Option 3: We can also solve this subgroups task with custom functions. | ||
|
|
||
| ```{r grouping_option_3} | ||
| format_agegr1 <- function(var_input) { |
There was a problem hiding this comment.
I would rename var_input to age. I think this is easier to read.
|
|
||
| ```{r demographics} | ||
| adsl_preds <- build_from_derived(metacore, | ||
| ds_list = list("dm" = dm), | ||
| predecessor_only = FALSE, keep = TRUE) | ||
| ds_list = list("dm" = dm_suppdm, "suppdm" = dm_suppdm), |
There was a problem hiding this comment.
I think an explanation would be helpful. Why is dm_suppdm specified twice?
There was a problem hiding this comment.
We combined [DM] and [SUPPDM] at the beginning with combine_supp() to show case this fantastic function.
In the specs it is specified from which domain the variables come from:
We then get the metacore object with the corresponding domain to look for. Since we combined [DM] and [SUPPDM], we need to specify that [DM] variables come from the dm_suppdm dataset that we just created, and [SUPPDM] variables also come from dm_suppdm.
Setting ds_list = list("dm" = dm_suppdm) only does not gather the variables from [SUPPDM]
If we want to separate ds_list = list("dm" = dm, "suppdm" = suppdm) we would need to transpose [SUPPDM] to get the parameters (QNAM) as variables.
There was a problem hiding this comment.
will try to think how to shorter the explanation without screenshots 🤓
|
|
||
| The other way to assign the parameter level values is using the `metatools` package with the `{metacore}` objects | ||
| that we created at the beginning. The function `metatools::create_var_from_codelist()` is used in below exemple. | ||
|
|
There was a problem hiding this comment.
Could we show the metadata?
Maybe we should also mention the requirements for using the metadata. As far as I remember the codelist can be either a pair of code/decode values or a list of permitted values. I think for using the metatools function code/decide pairs are needed.
@Fanny-Gautier I think this is definitely needed
Yes, primary goal is to show how the different packages work together. Secondary goal is to have more ready to go examples for teaching workshops. I think expanding the script is handy especially as @Fanny-Gautier has created a more robust specification file for the two datasets - we could handwave away/hide some of the derivations and say see the full script if it is indeed too wordy. TBD! Understood on we are repeating information here, but folks coming to this site I would think would like having the full script available to them rather than an abbreviated script and then have to go to admiral to get more derivations/information on that dataset. Maintaining alignment is something giving me anxiety. Regarding workshop goal - for ADSL we found a lot of stuff was pretty out of date and spec was pretty barebones. This update really brings stuff up to date for us. Hopefully, if someone wants to teach another workshop on programming ADSL from a spec to xpt file it will be almost 100% ready to go. Happy to keep discussing!! Thanks for the feedback |
|
Hi all, once all the above review comments are resolved I'm happy for this one to be approved/merged. I only had chance to review high level but I understand the perspective of having more "workshop-ready" examples, and this is something I also hear reflected in feedback from Posit colleagues. Thanks @Fanny-Gautier for all the efforts!! 🌟 |
|
@Fanny-Gautier as per chat today please name the specs file with "safety_specs" rather than ADSL, ADVS so that i can then add ADAE later in same file |
Co-authored-by: Stefan Bundfuss <[email protected]>
Co-authored-by: Stefan Bundfuss <[email protected]>
Pull Request
DESCRIPTION GOES HERE
Before you submit your pull request, take a look at the following checklist. Many thanks for your contribution!
Closes #<insert_issue_number>at the beginning of your PR title. Use the Edit button in the top-right if you need to update.DESCRIPTIONfile.DESCRIPTIONfile'sImportssection.