Closes #2415 replaced hard coding of expr(USUBJID, STUDYID) or expr(USUBJID) with … #2452

ProfessorP-beep · 2024-05-30T14:29:33Z

…!!!get_admiral_option("subject_keys") to allow for flexibility in using the function. There is an error in the bds_exposure.Rmd markdown on line 361, chunk 22 that needs to be fixed.

Thank you for your Pull Request! We have developed this task checklist from the Development Process Guide to help with the final steps of the process. Completing the below tasks helps to ensure our reviewers can maximize their time on your code as well as making sure the admiral codebase remains robust and consistent.

Please check off each taskbox as an acknowledgment that you completed the task or check off that it is not relevant to your Pull Request. This checklist is part of the Github Action workflows and the Pull Request will not be merged into the main branch until you have checked off each task.

…!!!get_admiral_option("subject_keys") to allow for flexibility in using the function. There is an error in the bds_exposure.Rmd markdown on line 361, chunk 22 that needs to be fixed.

github-actions · 2024-05-30T15:14:55Z

Package	Line Rate	Health
admiral	96%	✔
Summary	96% (4883 / 5082)	✔

bms63 · 2024-05-30T17:52:54Z

Looking nice @ProfessorP-beep! Can you just run styler on your code

…_option("subject_keys") in .Rmd files and ran each file to check for errors.

…e-in-all-functions

…yle error in pull request check

…nctions' of https://github.com/pharmaverse/admiral into 2415-general-issue-make-subject-keys-flexible-in-all-functions Merged pull request with main branch update

ProfessorP-beep · 2024-05-30T18:29:59Z

I'm still getting the error with pkgdown::build_site(), but everything else, including running the .Rmd checks out. It's running again right now after merging an update.

bms63 · 2024-05-30T18:31:13Z

I'm still getting the error with pkgdown::build_site(), but everything else, including running the .Rmd checks out. It's running again right now after merging an update.

I will see if I can build site on my end

bms63 · 2024-05-30T19:49:18Z

I was able to build website - I will review PR. @manciniedoardo can you also help with review please.

manciniedoardo

Left some comments, thanks @ProfessorP-beep !

manciniedoardo · 2024-05-31T08:58:22Z

inst/templates/ad_adae.R

@@ -40,7 +40,7 @@ adae <- ae %>%
 derive_vars_merged(
 dataset_add = adsl,
 new_vars = adsl_vars,
- by = exprs(STUDYID, USUBJID)
+ by = exprs(!!!get_admiral_option("subject_keys"))


This should be by_vars not by. Partial matching might save us here but better to use the right argument directly.

Also, I believe in this case you could do by_vars = get_admiral_option("subject_keys") directly rather than quoting and unquoting by_vars = exprs(!!!get_admiral_option("subject_keys")) (this is applicable in lots of other places as well.) You'd only need to use exprs and unquoting when you want to use other by vars as well, for instance by_vars = exprs(!!!get_admiral_option("subject_keys"), AEDECOD) - @bundfussr do you agree?

Maybe you could do a blanket find and replace for by_vars = exprs(!!!get_admiral_option("subject_keys")) -> by_vars = get_admiral_option("subject_keys")

Also, I believe in this case you could do by_vars = get_admiral_option("subject_keys") directly rather than quoting and unquoting by_vars = exprs(!!!get_admiral_option("subject_keys")) (this is applicable in lots of other places as well.) You'd only need to use exprs and unquoting when you want to use other by vars as well, for instance by_vars = exprs(!!!get_admiral_option("subject_keys"), AEDECOD) - @bundfussr do you agree?

I agree.

manciniedoardo · 2024-05-31T08:59:40Z

inst/templates/ad_adcm.R

@@ -36,7 +36,7 @@ adcm <- cm %>%
 derive_vars_merged(
 dataset_add = adsl,
 new_vars = adsl_vars,
- by = exprs(STUDYID, USUBJID)
+ by = exprs(!!!get_admiral_option("subject_keys"))


Again, should use by_vars not by

…emoved "!!!" from get_admiral_options as we do not neednto use unquoting where it is an argument

manciniedoardo · 2024-05-31T15:08:14Z

@ProfessorP-beep sorry, would it be possible to please update the occasions where we have exprs(get_admiral_option("subject_keys") to just say get_admiral_option("subject_keys")? I believe that would be the right syntax. thanks

ProfessorP-beep · 2024-05-31T16:59:28Z

@manciniedoardo Already done. I realized my mistake after that last push. New one coming as soon as the checks are done.

… Also ran styler() on ad.adcm.rm ad_adae, and occds.Rmd

…n last pushs

ProfessorP-beep · 2024-05-31T17:37:19Z

Sorry, hold off on reviewing on minute. Some of the changes weren't pushed through.

…k .R files

…ad_adppk.R")) to fix code style error

rossfarrugia · 2024-06-03T09:56:49Z

@bms63 any user could hardcode the subject identifiers instead if they were only ever going to use STUDYID, USBUJID here, but for our package to follow our own instructions at https://pharmaverse.github.io/admiral/reference/#admiral-options we have to follow the more generic way, otherwise an option that we ourselves advertise for users doesn't actually consistently work across the package.

bms63 · 2024-06-03T13:54:18Z

@bms63 any user could hardcode the subject identifiers instead if they were only ever going to use STUDYID, USBUJID here, but for our package to follow our own instructions at https://pharmaverse.github.io/admiral/reference/#admiral-options we have to follow the more generic way, otherwise an option that we ourselves advertise for users doesn't actually consistently work across the package.

Okay! I understand how options work :) I feel like I'm not making myself very clear with my concerns...so maybe my concerns for the users is moot! :) If you all think this update makes sense for the users, then full steam ahead!!

bms63 · 2024-06-03T13:55:55Z

and thanks @ProfessorP-beep for seeing this through!! This is how it works on the team :) We do PRs and I get a little huffy and puffy and ask funny questions - which usually are me being paranoid.

rossfarrugia · 2024-06-03T14:01:16Z

Thanks @bms63 - all fair questions, but i guess these all would have been discussed when we first added this option. Now that we have it offered in the package, we just need to make sure it works :)

bundfussr

There are a few missed occurrences:

(base) bundfuss@rstudio:(templates)> grep USUBJID *.R
ad_adlbhy.R:  select(STUDYID, USUBJID, TRT01A, PARAMCD, LBSEQ, ADT, AVISIT, ADY, AVAL, ANRHI, CRIT1, CRIT1FL)
ad_adlbhy.R:  select(STUDYID, USUBJID, TRT01A) %>% # add AVISIT, ADT for by visit
ad_adlbhy.R:  select(STUDYID, USUBJID, TRT01A, CRIT1FL, BILI_CRITFL) %>% # add AVISIT, ADT for by visit
ad_advs.R:  #   by_vars = exprs(STUDYID, USUBJID, PARAMCD, BASETYPE),

bundfussr · 2024-06-03T13:39:34Z

inst/templates/ad_adex.R

@@ -159,7 +159,7 @@ adex <- adex %>%
 )
 ),
 dataset_add = adex,
- by_vars = exprs(STUDYID, USUBJID, !!!adsl_vars)
+ by_vars = c(get_admiral_option("subject_keys"), exprs(!!!adsl_vars))


Suggested change

by_vars = c(get_admiral_option("subject_keys"), exprs(!!!adsl_vars))

by_vars = c(get_admiral_option("subject_keys"), adsl_vars)

Other occurrences should be updated as well.

Gotcha, I'll take a look at these. I think I was getting some error messages when using this with select(), but I'll look again and post it here if it comes up.

bundfussr · 2024-06-03T13:41:08Z

inst/templates/ad_adeg.R

@@ -165,7 +165,7 @@ adeg <- adeg %>%
 ) %>%
 # Derive QTLCR
 derive_param_qtc(
- by_vars = exprs(STUDYID, USUBJID, !!!adsl_vars, VISIT, VISITNUM, EGTPT, EGTPTNUM, ADTM, ADY),
+ by_vars = c(get_admiral_option("subject_keys"), exprs(!!!adsl_vars, VISIT, VISITNUM, EGTPT, EGTPTNUM, ADTM, ADY)),


Suggested change

by_vars = c(get_admiral_option("subject_keys"), exprs(!!!adsl_vars, VISIT, VISITNUM, EGTPT, EGTPTNUM, ADTM, ADY)),

by_vars = c(get_admiral_option("subject_keys"), adsl_vars, exprs(VISIT, VISITNUM, EGTPT, EGTPTNUM, ADTM, ADY)),

Other occurrences should be updated as well.

@ProfessorP-beep did this one need an update?

Oops looks like I missed it. Can update in a bit.

bundfussr · 2024-06-03T13:50:12Z

vignettes/bds_finding.Rmd

@@ -245,7 +245,7 @@ advs <- derive_vars_merged_lookup(
 ```{r, eval=TRUE, echo=FALSE}
 advs_param <- distinct(advs, USUBJID, PARAMCD, VSTESTCD)

-dataset_vignette(advs_param, display_vars = exprs(USUBJID, VSTESTCD, PARAMCD))
+dataset_vignette(advs_param, display_vars = exprs(!!get_admiral_option("subject_keys")[[2]], VSTESTCD, PARAMCD))


Either we should update line 246 as well or we should not update this line.

bundfussr · 2024-06-03T15:00:31Z

vignettes/generic.Rmd

@@ -219,7 +219,7 @@ advs <- tribble(
 derive_vars_merged(
 adsl,
 dataset_add = advs,
- by_vars = exprs(USUBJID),
+ by_vars = exprs(!!get_admiral_option("subject_keys")[[2]]),


This looks confusing to me. We don't have any rules or convention on the number of elements in "subject_keys" or which variable is placed where. You could have exprs(USUBJID, STUDYID), exprs(STUDYID, SITEID, SUBJID), or exprs(USUBJID). I.e., selecting a certain element is not a good idea because in general you don't know what you get.

I would either add STUDYID to the data and use get_admiral_option("subject_keys") instead of exprs(!!get_admiral_option("subject_keys")[[2]]) or just leave it as it is.

@bms63 , @rossfarrugia , what do you think?

add STUDYID to the data and use get_admiral_option("subject_keys") would be my preference. i don't like the [[2]] option either - as risky for future robustness.

This is being used in derive_var_trtemfl() as well. I flagged as concerning for me #2362 (comment)

Hi @ProfessorP-beep do you have time to make these final changes today?

Yea, I should be able to get to it this afternoon / early evening.

ProfessorP-beep · 2024-06-03T15:37:19Z

and thanks @ProfessorP-beep for seeing this through!! This is how it works on the team :) We do PRs and I get a little huffy and puffy and ask funny questions - which usually are me being paranoid.

No problem! I'm also learning a lot more so this is cool. I'm looking through the discussion now.

… can be used instead of subsetting with []. Replaced instances of !!!adsl_vars with just adsl_vars. Found other instances of USUBJID, STUDYID and replaced with get_admiral_optios("subject_keys") where errors didnt occur from the change.

…dmiral_options to USUBJID, STUDYID inside of select() functions to see if thats why scripts couldnt be executed

…ide the exprs() argument when defining by_vars

rossfarrugia · 2024-06-05T09:46:01Z

inst/templates/ad_adlbhy.R

@@ -43,7 +43,7 @@ adlb_annotated <- adlb %>%
 )
 )
 ) %>%
- select(STUDYID, USUBJID, TRT01A, PARAMCD, LBSEQ, ADT, AVISIT, ADY, AVAL, ANRHI, CRIT1, CRIT1FL)
+ select(USUBJID, STUDYID, TRT01A, PARAMCD, LBSEQ, ADT, AVISIT, ADY, AVAL, ANRHI, CRIT1, CRIT1FL)


why was this one left behind with just the order of USUBJID and STUDYID swapped? noticed this by chance as was using this PR to prompt same changes in admiralonco. please double check any others

as FYI should be select(!!!get_admiral_option("subject_keys"), TRT01A, ...etc)

bms63 · 2024-06-05T11:51:53Z

@bundfussr @rossfarrugia @manciniedoardo - This update isn't critical to me for 1.1. Can we implement for 1.2 as it is actually quite large and seems to be opening lots of questions?

manciniedoardo · 2024-06-05T13:21:17Z

@bundfussr @rossfarrugia @manciniedoardo - This update isn't critical to me for 1.1. Can we implement for 1.2 as it is actually quite large and seems to be opening lots of questions?

Ok, let's leave until 1.2 to ensure we do this right and don't have to come back to it later. I still think it's an important update to make, and we can definitely still leverage all the work done by @ProfessorP-beep in this PR, but what we have now is not wrong per se, just a bit inconsistent.

bms63 · 2024-06-05T19:50:13Z

Hi @ProfessorP-beep, we can talk about this tomorrow, but going to pause on this PR until after we ship 1.1 release. This will be our first PR into 1.2!

…ed, tested rmd and testthat files, but more test files should be addressed and tested where USUBJID and STUDYID are used.

ProfessorP-beep · 2024-06-06T15:10:21Z

Hi @ProfessorP-beep, we can talk about this tomorrow, but going to pause on this PR until after we ship 1.1 release. This will be our first PR into 1.2!

Sounds good, I'm just gonna push the additional changes I've done since then. There is probably still some review to be done to make sure all cases are addressed.

…e-in-all-functions

…verted the files back to their original script. There is a missing dependency import for "glue"

github-actions · 2024-06-22T02:13:00Z

This Pull Request is stale because it has not been worked on in 15 days.

github-actions · 2024-07-09T02:22:34Z

This Pull Request is stale because it has not been worked on in 15 days.

replaced hard coding of expr(USUBJID, STUDYID) or expr(USUBJID) with …

941fa3a

…!!!get_admiral_option("subject_keys") to allow for flexibility in using the function. There is an error in the bds_exposure.Rmd markdown on line 361, chunk 22 that needs to be fixed.

ProfessorP-beep requested a review from jeffreyad as a code owner May 30, 2024 14:29

ProfessorP-beep linked an issue May 30, 2024 that may be closed by this pull request

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

Open

ProfessorP-beep changed the title ~~replaced hard coding of expr(USUBJID, STUDYID) or expr(USUBJID) with …~~ #2415 replaced hard coding of expr(USUBJID, STUDYID) or expr(USUBJID) with … May 30, 2024

ProfessorP-beep added 2 commits May 30, 2024 10:43

merged with changes from origin main

b0f3e0c

merged with changes from origin main

fd5a292

ProfessorP-beep and others added 4 commits May 30, 2024 14:12

replaced expr(STUDYID, USUBJID) and expr(USUBJID) with !!!get_admiral…

cb3363e

…_option("subject_keys") in .Rmd files and ran each file to check for errors.

Merge branch 'main' into 2415-general-issue-make-subject-keys-flexibl…

66a98fb

…e-in-all-functions

ran styler::style_file("inst/templates/ad_adex.R") to address Code St…

84794a1

…yle error in pull request check

Merge branch '2415-general-issue-make-subject-keys-flexible-in-all-fu…

55f8b81

…nctions' of https://github.com/pharmaverse/admiral into 2415-general-issue-make-subject-keys-flexible-in-all-functions Merged pull request with main branch update

ProfessorP-beep requested a review from bms63 May 30, 2024 18:28

bms63 requested a review from manciniedoardo May 30, 2024 19:49

manciniedoardo requested changes May 31, 2024

View reviewed changes

ProfessorP-beep added 2 commits May 31, 2024 10:34

change instances of by = get_admiral_option(...) to by_vars =. Also r…

2157bba

…emoved "!!!" from get_admiral_options as we do not neednto use unquoting where it is an argument

Updated News.md

e2638ed

ProfessorP-beep added 2 commits May 31, 2024 13:14

fixed syntax and remoced expr() from around get_admiral_options(...).…

80b9cf2

… Also ran styler() on ad.adcm.rm ad_adae, and occds.Rmd

styler function ran on templates wasnt in last push

3628b0b

ProfessorP-beep requested a review from manciniedoardo May 31, 2024 17:26

Missed some get_admiral_option syntax changes that werentt included i…

b56823f

…n last pushs

ProfessorP-beep added 3 commits May 31, 2024 19:43

Addressed additional get_admiral_option(...) syntax issues

61fd719

Fixed errors due to missing parentheses in ad_adex, ad_admh, and adpp…

7f00946

…k .R files

ran styler::style_file(c("inst/templates/ad_adex.R", "inst/templates/…

f51d7be

…ad_adppk.R")) to fix code style error

bundfussr reviewed Jun 3, 2024

View reviewed changes

rossfarrugia mentioned this pull request Jun 4, 2024

Documentation & Templates: Make subject keys consistently flexible pharmaverse/admiralophtha#226

Closed

ProfessorP-beep added 4 commits June 4, 2024 16:01

ran styler_file() on changed vignettes in this commit

d0d9aa1

ran styler_file() on changed vignettes in this commit. Reverted get_a…

badf8c9

…dmiral_options to USUBJID, STUDYID inside of select() functions to see if thats why scripts couldnt be executed

made changes to !!!adsl_vars to remove exclamations and bring it outs…

27dc85d

…ide the exprs() argument when defining by_vars

rossfarrugia reviewed Jun 5, 2024

View reviewed changes

addressed additional instances where get_admiral_options should be us…

8573bdc

…ed, tested rmd and testthat files, but more test files should be addressed and tested where USUBJID and STUDYID are used.

ProfessorP-beep and others added 7 commits June 6, 2024 11:10

Merge branch 'main' into 2415-general-issue-make-subject-keys-flexibl…

39de758

…e-in-all-functions

updated wordlist with spellcheck package

e379b8f

resolving merge to update code

d679577

additional USUBJID and STUDYID fixes

6932070

received some errors in devtools::check() that I investigated then re…

bd1378f

…verted the files back to their original script. There is a missing dependency import for "glue"

addressed missed replacements in inst/templates scripts

5b975ff

Ran spellcheck, lintr, and devtools to address pull request errors.

50c2dd7

github-actions bot added the stale label Jun 22, 2024

bms63 removed the stale label Jun 23, 2024

github-actions bot added the stale label Jul 9, 2024

rossfarrugia mentioned this pull request Jul 9, 2024

Closes #59 derive params growth age add interpolation pharmaverse/admiralpeds#60

Merged

14 tasks

	by_vars = c(get_admiral_option("subject_keys"), exprs(!!!adsl_vars))
	by_vars = c(get_admiral_option("subject_keys"), adsl_vars)

Closes #2415 replaced hard coding of expr(USUBJID, STUDYID) or expr(USUBJID) with … #2452

Are you sure you want to change the base?

Closes #2415 replaced hard coding of expr(USUBJID, STUDYID) or expr(USUBJID) with … #2452

Conversation

ProfessorP-beep commented May 30, 2024 • edited Loading

github-actions bot commented May 30, 2024 • edited Loading

bms63 commented May 30, 2024

ProfessorP-beep commented May 30, 2024

bms63 commented May 30, 2024

bms63 commented May 30, 2024

manciniedoardo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manciniedoardo commented May 31, 2024

ProfessorP-beep commented May 31, 2024

ProfessorP-beep commented May 31, 2024

rossfarrugia commented Jun 3, 2024

bms63 commented Jun 3, 2024

bms63 commented Jun 3, 2024 • edited Loading

rossfarrugia commented Jun 3, 2024

bundfussr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ProfessorP-beep commented Jun 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bms63 commented Jun 5, 2024

manciniedoardo commented Jun 5, 2024

bms63 commented Jun 5, 2024

ProfessorP-beep commented Jun 6, 2024

github-actions bot commented Jun 22, 2024

github-actions bot commented Jul 9, 2024

ProfessorP-beep commented May 30, 2024 •

edited

Loading

github-actions bot commented May 30, 2024 •

edited

Loading

bms63 commented Jun 3, 2024 •

edited

Loading