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.

Sign up for GitHub

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

Jump to bottom

Feature Request: Implement derive_vars_crit_flag() #2468

Closed
bundfussr opened this issue Jun 21, 2024 · 7 comments · Fixed by #2469 · May be fixed by #2479
Closed

Feature Request: Implement derive_vars_crit_flag() #2468

bundfussr opened this issue Jun 21, 2024 · 7 comments · Fixed by #2469 · May be fixed by #2479
Assignees
@bundfussr
Labels
enhancement New feature or request programming

Comments

@bundfussr
Copy link
Collaborator

bundfussr commented Jun 21, 2024 •
edited
Loading

Feature Idea

Implement derive_vars_crit_flag() which derives the variables CRITy, CRITyFL, and CRITyFLN (for a specific value of y). The derivation of these variables is very simple. However, there are some requirements which many programmers and specs authors are not aware of. For example, that the derivation must rely on variables from the current records only and the relation of the values of CRITy and CRITyFL (e.g., if CRITyFL is set to "Y" or "N", CRITy must be populated for all records with the same value).

The function should cover all use cases of the CRIT variables. I.e., if a CRIT variable can not be derived with the function, the derivation is not ADaM compliant.

Relevant Input

No response

Relevant Output

No response

Reproducible Example/Pseudo Code

adxx <- adxx %>% derive_vars_crit_flag(
  crit_nr = 1,
  condition = AVAL > 50,
  description = paste(PARAM, "> 50"),   # CRITy is set to the specified value
  values_yn = FALSE,                    # should values "Y", "N", and NA be used?
  create_numeric_flag = TRUE            # should CRITyFLN be created?
)
@bundfussr bundfussr added enhancement New feature or request programming labels Jun 21, 2024
@bms63
Copy link
Collaborator

bms63 commented Jun 23, 2024

I think this looks useful!

Do we want to use _flag at the end to keep in line with other flag related functions?

Also, we have the hy's law vignette which discusses Crit flags - maybe opportunity to update with this function?

https://pharmaverse.github.io/admiral/articles/hys_law.html

What do you think @pharmaverse/admiral and @pharmaverse/admiral_comm? Any takers? I think this could be a fun one to implement for those eager to do some function building for admiral!!

@bundfussr
Copy link
Collaborator Author

Do we want to use _flag at the end to keep in line with other flag related functions?

Yes, seems reasonable to me.

@bundfussr bundfussr changed the title Feature Request: Implement derive_vars_crit() Feature Request: Implement derive_vars_crit_flag() Jun 24, 2024
@manciniedoardo
Copy link
Collaborator

manciniedoardo commented Jun 24, 2024 •
edited
Loading

Looks reasonable to me as well, thanks for the suggestion @bundfussr.

Just for general awareness, in admiralophtha we have derive_var_bcvacrtixfl() - see here for reference. But to be honest I don't think this function is totally in line with our {admiral} API so I wouldn't mind absorbing its functionality inside a new {admiral} function and then either superseding it or turning it into a wrapper for the {admiral} function.

On another note, could we couple this effort with a similar function for AVALCATx?

@bundfussr
Copy link
Collaborator Author

On another note, could we couple this effort with a similar function for AVALCATx?

@manciniedoardo , what do you have in mind? Should it derive AVALCATx only or also similar variables like CHGCATy or AGEGRy? What is expected as input?

@bundfussr bundfussr self-assigned this Jun 24, 2024
@manciniedoardo
Copy link
Collaborator

On another note, could we couple this effort with a similar function for AVALCATx?

@manciniedoardo , what do you have in mind? Should it derive AVALCATx only or also similar variables like CHGCATy or AGEGRy? What is expected as input?

Actually it was just a passing thought, I haven't looked in the IG yet to see if AVALCAT/CHGCAT have as stringent guidelines. My thought was that if yes, maybe we could also develop derive_var_avalcat() and similar.

@nbrucken17
Copy link

AVALCATy has a restriction that it can only be derived using values of AVAL or AVALC. CHGCATy is under a similar restriction- it can only be derived using values of CHG. AGEGRy should only be derived based on values of age, but which age variable is used is up to the producer, since multiple age variables are possible in ADSL.

@manciniedoardo
Copy link
Collaborator

AVALCATy has a restriction that it can only be derived using values of AVAL or AVALC. CHGCATy is under a similar restriction- it can only be derived using values of CHG. AGEGRy should only be derived based on values of age, but which age variable is used is up to the producer, since multiple age variables are possible in ADSL.

Thanks @nbrucken17 - if that is all maybe this is not a good idea, as you may just as well derive them inside a mutate statement!

bundfussr added a commit that referenced this issue Jun 24, 2024
@bundfussr
#2468 derive_vars_crit_flag: implement derive_vars_crit_flag() and tests
8740df9
bundfussr added a commit that referenced this issue Jun 25, 2024
@bundfussr
#2468 derive_vars_crit_flag: handle invalid expressions
65f7e33
bundfussr added a commit that referenced this issue Jun 25, 2024
@bundfussr
#2468 derive_vars_crit_flag: add documentation
09de412
bundfussr added a commit that referenced this issue Jun 26, 2024
@bundfussr
#2468 derive_vars_crit_flag: update templates and vignettes
bff432d
@bundfussr bundfussr linked a pull request Jun 26, 2024 that will close this issue
Closes #2468 Implement derive_vars_crit_flag() #2469
Merged
15 tasks
bundfussr added a commit that referenced this issue Jun 26, 2024
@bundfussr
#2468 derive_vars_crit_flag: style files
159b1d8
bundfussr added a commit that referenced this issue Jun 26, 2024
@bundfussr
#2468 derive_vars_crit_flag: fix lintr
ef3efbd
bundfussr added a commit that referenced this issue Jun 26, 2024
@bundfussr
#2468 derive_vars_crit_flag: update cheatsheet
26bb1cc
bundfussr added a commit that referenced this issue Jul 9, 2024
@bundfussr
#2468 derive_vars_crit_flag: add example to bds_finding vignette
38fdf63
bundfussr added a commit that referenced this issue Jul 9, 2024
@bundfussr
#2468 derive_vars_crit_flag: add details to test decription and add r…
36ae276 
…easoning to documentation
@bms63 bms63 closed this as completed in 4cd9cfd Jul 9, 2024
@bundfussr bundfussr mentioned this issue Jul 15, 2024
Open
3 tasks
bundfussr added a commit that referenced this issue Jul 15, 2024
@bundfussr
#2468 derive_vars_crit_flag: add 1.1.1 updates to cheatsheet
3be2122
@bundfussr bundfussr linked a pull request Jul 15, 2024 that will close this issue
Closes #2468 derive_vars_crit_flag: add 1.1.1 updates to cheatsheet #2479
Draft
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bundfussr bundfussr

Labels
enhancement New feature or request programming
Projects
admiral (sdtm/adam, dev, ci, template, core)
Status: Archive
Milestone
No milestone
4 participants
@bms63 @manciniedoardo @nbrucken17 @bundfussr