Decision on which "questioning" functions to deprecate?

[rossfarrugia]

Feature Idea

See Stefan's comment in #936. We have lots of questioning badge functions that it might be nice to clean up for this release and decide to either keep or deprecate. Here's a proposal from my perspective and @bms63 & @thomas-neitmann feel free to make the final call:

• derive_var_ady - we should deprecate in favour of derive_vars_dy as we mention this simple example even in our new manifesto
• derive_var_astdy - as above
• derive_var_aendy - as above
• derive_var_trtedtm - personally i would leave it, as i think it's fine to have a more targeted function for something as common as this to make it easier for users to not have to figure out all the inputs to the more generic function like derive_vars_merged_dtm. So I would say not to deprecate. I see the difference with the above study day cases being that study day is so much simpler & i like that the filter_ex default gives you a starter for common code you could use to filter out invalid doses.
• derive_var_trtsdtm - as above
• derive_var_disposition_dt - i personally would deprecate in favour of derive_vars_merged_dt now, as we basically have 2 quite generic functions very similar but one is only for any DS date whereas the other can be used on any domain
• derive_var_lstalvdt - already deprecated in devel via https://github.com/pharmaverse/admiral/pull/959 in favour of derive_var_extreme_dt

Remember the impact to the ADaM vignettes and templates.

If a new contributor picks this up later once decision made, then reminder to use: https://pharmaverse.github.io/admiral/articles/programming_strategy.html#deprecation-1

Relevant Input

NA

Relevant Output

NA

Reproducible Example/Pseudo Code

NA

[bundfussr]

I would deprecate derive_var_trtsdtm() and derive_var_trtedtm(). Especially the default for filter_ex seems dangerous to me because it hides that doses with incomplete date are excluded. I think it should be a conscious decision of the programmer which exposure observations are excluded. For example, in an interim analysis EXENDTC may be missing because the treatment is ongoing. With the current default this observation would be excluded and the treatment end date may be set to a date before data cut off date. This may result in ignoring adverse events close to the data cut off date in safety summaries.

If incomplete dates are not excluded, the functions impute the dates (which is not documented) but do not derive the imputation flags, which are required according CDISC. It is also not possible to control the imputation, e.g., specify preserve = TRUE or max_dates = vars(DCUTDT). In these cases derive_vars_merged_dtm() must be used.

Thus I would deprecate derive_var_trtsdtm() and derive_var_trtedtm() in favor of derive_vars_merged_dtm() and extend the "Derive/Impute Numeric Treatment Date/Time and Duration (TRTSDTM, TRTEDTM, TRTDURD)" section in the ADSL vignette to discuss exclusion of invalid doses and imputation and provide examples.

[bundfussr]

@bms63 , @thomas-neitmann , how should we proceed with this issue? Should we set it on the agenda for next Monday, have a separate meeting, or something else?

[bms63]

1056 I think @amsmith214 is also tackling this. I'm happy to do a separate meeting so we can get us all on the same page. I can not make Q/C Meetings as I have a study meeting at that time.

[rossfarrugia]

@bms63 i think these are 2 separate issues. 1056 as i understood it talks about completely removing functions that have long been deprecated in previous releases, so that no need to even show them in our documentation or code anymore. whereas this issue talks of functions to potentially deprecate as part of this next release, which in time then likely get completely removed as part of a later release.

i would say it should be yours and @thomas-neitmann call as technical leads as to which functions to deprecate with each release, so i trust you both to make the final call. Stefan's points above make sense to me, so I don't have a strong opinion either way, as long as it is all well explained in the ADSL vignette with meaningful and realistic examples given there.

[thomas-neitmann]

@bms63 and I decided to deprecate all function listed by @rossfarrugia above.

[rossfarrugia]

thanks @thomas-neitmann & @bms63 - happy if you want to close this issue and make a new issue with the required steps to do the deprecations like reminding to update the ADaM templates and vignettes etc, or use this issue and get someone new signed up to it. if we include enough detail i'm sure a non-core contributor could help pick up this action

[bundfussr]

I'll start the implementation of deprecating the functions now.

[rossfarrugia]

thanks @bundfussr - sure you'd spot it, but i noticed even pages like the get started https://pharmaverse.github.io/admiral/articles/admiral.html will be impacted as it includes:

  derive_var_trtsdtm(dataset_ex = ex) %>%
  derive_var_trtedtm(dataset_ex = ex) %>%