Closed rossfarrugia closed 3 days ago
@pharmaverse/admiral @pharmaverse/admiral_comm this is a good one to dip your toes into admiral development!
@ProfessorP-beep
Yea, I can grab this.
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)
)
Can we also please review the vignettes and templates where this is applicable? From a cursory look I found cases in:
... 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.
@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
@manciniedoardo I started working on it this morning. I think I can still get it done.
Hi @ProfessorP-beep any progress?
We have a release planned for Monday, June 3rd
@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 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?
@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.
I may have fixed it. Going to try building again.
@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?
@bms63 Sounds good to me.
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.
Background Information
@bundfussr did a review of the admiral functions and found 2 occurrences where
STUDYID
andUSUBJID
are hardcoded instead of usingget_admiral_option("subject_keys")
as per https://pharmaverse.github.io/admiral/reference/#admiral-options.These are
derive_vars_atc()
andcreate_single_dose_dataset()
, for example: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 ofUSUBJID
.