General Issue: Make subject keys flexible in all functions

[rossfarrugia]

Background Information

@bundfussr did a review of the admiral functions and found 2 occurrences where STUDYID and USUBJID are hardcoded instead of using get_admiral_option("subject_keys") as per https://pharmaverse.github.io/admiral/reference/#admiral-options.

These are derive_vars_atc() and create_single_dose_dataset(), for example:

derive_vars_atc <- function(dataset,
                            dataset_facm,
                            by_vars = exprs(USUBJID, CMREFID = FAREFID),
                            value_var = FASTRESC)

Definition of Done

These functions should be flexible so as if ever set_admiral_options(subject_keys = exprs(STUDYID, USUBJID2)) was set by user then these variables would be used everywhere instead of USUBJID.

[manciniedoardo]

@pharmaverse/admiral @pharmaverse/admiral_comm this is a good one to dip your toes into admiral development!

[bms63]

@ProfessorP-beep

[ProfessorP-beep]

Yea, I can grab this.

[rossfarrugia]

thanks @ProfessorP-beep - please also sanity check if any functions have examples of this within the assertion checks, such as:

  assert_data_frame(
    dataset,
    required_vars = exprs(STUDYID, USUBJID, ...etc)
  )

which should be replaced by:

  assert_data_frame(
    dataset,
    required_vars = exprs(!!!get_admiral_option("subject_keys"), ...etc)
  )

[manciniedoardo]

Can we also please review the vignettes and templates where this is applicable? From a cursory look I found cases in:

• occds vignette
• higer_order vignette
• admiral vignette
• visit_periods vignette
• bds_finding vignette
• bds_tte vignette
• adsl vignette
• bds_exposure vignette
• questionnaires vignette
• pk_adnca vignette
• generic vignette
• adppk vignette
• adlbhy template
• adsl template
• adcm template
• adpc template
• adppk template
• adae template
• advs template
• admh template
• adlb template
• adex template
• adeg template
• adpp template

... and this is just where I found by_vars = exprs(USUBJID) or by_vars = exprs(STUDYID, USUBJID) . We can also address cases such as the ones in the above comment by @rossfarrugia where there are other by variables.

I think when it appears in roxygen2 templates that's ok as our examples should be self-contained.

[manciniedoardo]

@ProfessorP-beep how is this going? If you would like to hand this over please feel free, as we know you are also working on this issue

[ProfessorP-beep]

@manciniedoardo I started working on it this morning. I think I can still get it done.

[bms63]

Hi @ProfessorP-beep any progress?

[bms63]

We have a release planned for Monday, June 3rd

[ProfessorP-beep]

@bms63 I got all the instances that were hard coded. I just ran into an error building the package on bds_exposure.Rmd

That I was looking at yesterday. Looks like its in something with admiral::derive_param_exposure(...)

Error: 
! in callr subprocess.
Caused by error in `map(.x, .f, ..., .progress = .progress)`:
! ℹ In index: 3.
Caused by error in `render_rmarkdown()`:
! Failed to render RMarkdown document.
✖ Quitting from lines at lines 362-373
  [unnamed-chunk-22] (bds_exposure.Rmd)
Caused by error:
! in callr subprocess.
Caused by error:
! Argument `enexpr(analysis_var)` must be
  a <symbol>, but is missing.
ℹ See `$stdout` and `$stderr` for standard output and error.

[bms63]

@bms63 I got all the instances that were hard coded. I just ran into an error building the package on bds_exposure.Rmd

That I was looking at yesterday. Looks like its in something with admiral::derive_param_exposure(...)

Error: 
! in callr subprocess.
Caused by error in `map(.x, .f, ..., .progress = .progress)`:
! ℹ In index: 3.
Caused by error in `render_rmarkdown()`:
! Failed to render RMarkdown document.
✖ Quitting from lines at lines 362-373
  [unnamed-chunk-22] (bds_exposure.Rmd)
Caused by error:
! in callr subprocess.
Caused by error:
! Argument `enexpr(analysis_var)` must be
  a <symbol>, but is missing.
ℹ See `$stdout` and `$stderr` for standard output and error.

Maybe something is wrong with the data source? I think there has been some updates in pharmaversesdtm?

[ProfessorP-beep]

@bms63 I have an active pull request for this if anyone also wants to take a look. I'll recheck what I've done and ensure I have updated packages and up to date with the main branch. I saw a couple of Rmd files that still had expr(USUBJID), so I will make sure those are all sorted as well.

[ProfessorP-beep]

I may have fixed it. Going to try building again.

[bms63]

@manciniedoardo this got a little messy and we decided to close the PR.

Could we rethink the scope of this issue? Looking at what Ross requested - it was just for the two functions. The vignettes and template updates seem to open a can of worms.

Could we start by updating the functions first and then create separate issues for the templates and vignettes?

[manciniedoardo]

@bms63 Sounds good to me.

[bms63]

I've created another issue for the "original issue", i.e. updating the two functions.

I put it on this week's agenda to discussion updating vignettes and tempaltes.