Deprecate derive_vars_suppqual

[rossfarrugia]

Deprecate https://github.com/pharmaverse/admiral/blob/main/R/derive_vars_suppqual.R once @statasaurus adds the functionality to metatools, and update any vignettes mentioning this to take it out and we can cover instead in the e2e ADaM creation vignette on the pharmaverse site.

Rationale: Adding/removing suppquals to/from SDTM is not strictly part of ADaM creation, and can be needed for other purposes, such as preparing SDTM for eSub. So we felt this fits better into metatools instead and Christina kindly offered to help support the development effort with @mstackhouse (FYI Mike as you proposed this over email). p.s. i'll remove the SUPPQUAL repo i created in pharmaverse, as was a bad idea in hindsight to have a separate new package for such small functionality.

[bundfussr]

@statasaurus , tester feedback regarding derive_vars_suppqual():

I can not generate the data for my own protocol dm and suppdm although I run package sample data dm without error. It is due to the fact the Oncology trial SUPPDM data we have have IDVAR and IDVARVAL populated USUBJID and its value besides variable STUDYID/DOMAIN/USUBJID. The current admiral function gives error. Suggested solution: expand function ability to not assume IDVAR and IDVARVAL empty. They could USUBJID and its values.

Can you consider this in your implementation?

[bms63]

Do we want to get this into metatools and start the deprecation process for the upcoming CRAN release

[rossfarrugia]

@bms63 - it's there now https://pharmaverse.github.io/metatools/reference/combine_supp.html and i have someone from Roche that has done some user testing and fed back to @statasaurus & @mstackhouse.

Christina could comment as to whether now is a good time to get this out of admiral. The pharmaverse e2e vignette will then help to explain where to get this utility from.

[bms63]

This will be interesting as we have only deprecated functions that are replaced by other functions within admiral. Now we are deprecating for something outside admiral, i.e. metatools. I imagine this will happen some more as more packages come online and we shuffle stuff around.

Does this become metatools::derive_vars_suppqual ?

Anything else could be problematic for someone picking this up?

[statasaurus]

whether now is a good time to get this out of admiral. The pharmaverse e2e vignette will then help to explain where to get this utility from.

Yes you can! I was waiting on the art for metacore. But that is done. So I am probably push to CRAN for that today and metatools by the end of the week or so

[rossfarrugia]

@bms63 for the metatools::derive_vars_suppqual point, we wouldn’t want to tie in metatools as a package dependency - it should be optional to use alongside admiral. So my thinking would be just to retire the function completely, remove mention from the vignettes and explain this in the changelog. Then in the pharmaverse e2e vignette we would cover this step. Does that sound ok or shout if I'm missing anything?

[bms63]

Umm...I would think some sort of messaging would be nice - beyond just the changelog. Does anyone actually check that besides developers :) ?

Also, thinking of users....for example I'm using it in my code for datasets at GSK and if I installed 0.7.0 and the update crashed my code with no messaging then I would be a bit clueless.

[rossfarrugia]

Fair, makes sense - agree to give a user message but i'd say no need to then automatically route the code to the metatools function - that was more my point here. a user would need to make that call whether they want to use this new package (and if using on a validated container they'd need to get validated as per their company requirements etc).

Also i'd say a changelog is like a bible for any user too, especially when the package is still evolving so quickly like this one.

[rossfarrugia]

i see now you were suggesting to show metatools in the documentation/roxygen2 section rather than the code itself - i'm fine with that.

i was saying i dont think we need it in the usual code section we add:

fun_xxx <- function(dataset, new_var) {
  deprecate_warn("x.y.z", "fun_xxx()", "new_fun_xxx()")
  new_fun_xxx(dataset, new_var = new_var)
}

[thomas-neitmann]

In order to avoid a metatools dependency let's not use the usual

fun_xxx <- function(dataset, new_var) {
  deprecate_warn("x.y.z", "fun_xxx()", "new_fun_xxx()")
  new_fun_xxx(dataset, new_var = new_var)
}

but rather

fun_xxx <- function(dataset, new_var) {
  deprecate_stop("x.y.z", "fun_xxx()", "metatools::new_fun_xxx()")
}

[millerg23]

Hi @statasaurus
Are you OK if I make the changes to {admiral} discussed previously in this ticket, assume everything is good from metatools side?