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

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
Labels
enhancement New feature or request programming

Comments

@bundfussr
Copy link
Collaborator

bundfussr commented Jun 21, 2024

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?
)
@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

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
@bundfussr bundfussr moved this from Backlog to In Progress in admiral (sdtm/adam, dev, ci, template, core) 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 bundfussr linked a pull request Jun 26, 2024 that will close this issue
15 tasks
bundfussr added a commit that referenced this issue Jul 9, 2024
@bms63 bms63 closed this as completed in 4cd9cfd Jul 9, 2024
@bundfussr bundfussr linked a pull request Jul 15, 2024 that will close this issue
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment