Feature Request: Implement `derive_vars_crit_flag()`

bundfussr commented 1 month ago

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 commented 1 month ago

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 commented 4 weeks ago

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

Yes, seems reasonable to me.

manciniedoardo commented 4 weeks ago

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 commented 4 weeks ago

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?

manciniedoardo commented 4 weeks ago

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 commented 4 weeks ago

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 commented 4 weeks ago

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!

pharmaverse / admiral