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.

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

General Issue: Make subject keys flexible in all functions #2415

Closed
rossfarrugia opened this issue Apr 23, 2024 · 16 comments
Closed

General Issue: Make subject keys flexible in all functions #2415

rossfarrugia opened this issue Apr 23, 2024 · 16 comments
Assignees

Comments

@rossfarrugia
Copy link
Collaborator

rossfarrugia commented Apr 23, 2024

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
Copy link
Collaborator

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

@bms63
Copy link
Collaborator

bms63 commented Apr 23, 2024

@ProfessorP-beep

@ProfessorP-beep
Copy link
Collaborator

Yea, I can grab this.

@ProfessorP-beep ProfessorP-beep self-assigned this Apr 23, 2024
@rossfarrugia
Copy link
Collaborator Author

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
Copy link
Collaborator

manciniedoardo commented Apr 23, 2024

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
Copy link
Collaborator

@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
Copy link
Collaborator

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

@bms63
Copy link
Collaborator

bms63 commented May 30, 2024

Hi @ProfessorP-beep any progress?

@bms63
Copy link
Collaborator

bms63 commented May 30, 2024

We have a release planned for Monday, June 3rd

@ProfessorP-beep
Copy link
Collaborator

ProfessorP-beep commented May 30, 2024

@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
Copy link
Collaborator

bms63 commented May 30, 2024

@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
Copy link
Collaborator

ProfessorP-beep commented May 30, 2024

@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
Copy link
Collaborator

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

@bms63
Copy link
Collaborator

bms63 commented Aug 21, 2024

@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
Copy link
Collaborator

@bms63 Sounds good to me.

@bms63
Copy link
Collaborator

bms63 commented Sep 3, 2024

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.

@bms63 bms63 closed this as completed Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants