General Issue: Clean-up Round 2

[bundfussr]

Background Information

To achieve a "stable" admiral version we need a clean-up of admiral, i.e.,

• avoid that functionality of functions overlaps
• name function and arguments consistently
• close gaps in functionality

This issue is intended to collect functions which should be revised and identify gaps.

@pharmaverse/admiral , please feel free to add.

Definition of Done

No response

[bundfussr]

From @kaz462 :

There are four functions for deriving extreme event/record as a new parameter, the function names are a bit confusing to me when I try to compare them: _record(s)/_event, with/without _param

	Data source: one or multiple	Extreme is ordered by -	filter/dataset source argument name
1.derive_extreme_records	one	order argument	filter/
2.derive_extreme_event	one	the order of events entered and the order argument
3.derive_param_extreme_event	one	order argument	filter_source/dataset_source
4.derive_param_extreme_record	multiple	order argument	/source_datasets

• What do you think of combining some functions? e.g.,

  • function 3 is a special case of function 2

  • function 4 is an extension of function 2 by allowing multiple data sources

  • function 1 and 3 has been combined through Closes #1725 Combine derive_extreme_records() and derive_param_extreme_event() #1882

• Should we make some argument name consisnent across those functions?

[bundfussr]

Should all flag functions provide true_vlue and false_value argument?

[zdz2101]

I actually don't think its necessary, feels like if they wanted to modify it from the usual "Y" values, its a really simple dplyr::mutate statement they can address on their own as a post-processing step, the more arguments we add to functions only adds more complexity to our codebase

[bundfussr]

derive_summary_records() should provide the dataset_add argument.
Consider the following use case.

adae <- adae %>%
  mutate(AVAL = 1)

adsaftte_aetot <- derive_summary_records(
  adae,
  filter = ANL01FL == "Y",
  set_values_to = exprs(
    PARAMCD = "AETOT1",
    PARAM = "Number of occurrences of any adverse event",
    PARCAT1 = "Total Occurrences",
    ANL01FL = "Y"
  ),    
  by_vars = exprs(STUDYID, USUBJID),
  analysis_var = AVAL,
  summary_fun = sum
) %>%
  filter(PARAMCD == "AETOT1") %>%
  select(STUDYID, USUBJID, PARAMCD, PARAM, PARCAT1, AVAL, ANL01FL)

The filter() and select() call at the end could be omitted if the dataset_add argument is available.

[bundfussr]

The arguments of derive_vars_joined(), derive_var_joined_exist_flag(), and filter_joined() should be made consistent. For example, all functions should provide the dataset_add argument.

[zdz2101]

@bms63 should we actually turn this into discussion with the respective individual pieces of consolidation as their own issues?

Link to mural of function arguments for viewing: https://app.mural.co/t/admiraldevelopmentteam6136/m/admiraldevelopmentteam6136/1689188235153/04c3276047791dc443bc22046e0804924a1e6137?sender=u2186aa07ff6a351a07a19987

[zdz2101]

Many of our function arguments serve similar/same purposes but have different names, here are some recommended swaps, we can modify the table such that by the end/after all talk it becomes the "Definition of done"

argument	function	recommended	implemented?	Strategy Addition/Update	All Done?
key_vars	consolidate_metadata	by_vars
check_vars	consolidate_metadata	check_type	Questioning
check_keys	consolidate_metadata	check_unqiue		Needed
start_datetime	create_single_dose_dataset	start_date	Will Not	Needed
end_datetime	create_single_dose_dataset	end_date	Will Not	Needed
dataset_expected_obs	derive_expected_records	dataset_ref	#2037	#306
dataset_expected_obs	derive_locf_records	dataset_ref	#2037	#306
dataset_adsl	derive_param_exist_flag	dataset_add	Already Done	#306
dataset_adsl	derive_param_extreme_event	dataset_add	Being Deprecated	Needed
dataset_adsl	derive_param_tte	dataset_add	Most likely leave as is	#306
na_val	derive_var_shift	missing_value	#2014	#296
trt_start_date	derive_var_trtemfl	start_date	Will Not	Needed
trt_end_date	derive_var_trtemfl	end_date	Will Not	Needed
dataset_facm	derive_vars_atc	dataset_add	Will Not	Needed
reference_date	derive_vars_dy	start_date	#2027
missing_values	derive_vars_joined	missing_value	Leave as is	#296
missing_values	derive_vars_merged	missing_value	Leave as is	#296
dataset_merge	derive_vars_transposed	dataset_add	Needed
cond	max_cond	condition
cond	min_cond	condition
wt	compute_egfr	weight	#2020
.../args	lots of places	sources	#2023

[bundfussr]

We can not rename trt_start_date and trt_end_date in derive_var_trtemfl() because start_date and end_date are already arguments of the function.

[zdz2101]

I did notice that, will remove

[bms63]

Made additional column for updating common arguments. Hoping this helps for when new functions are created we can cross-check with common arguments.

https://pharmaverse.github.io/admiraldev/devel/articles/programming_strategy.html#list-of-common-arguments

[bms63]

Check yourself before you wreck yourself

To me check_vars is doing something different from check_type and I think should be left as is.

consolidate_metadata

However,

we should bring the check_keys in line with the check_type, e.g. filter_extreme check_type is below. Perhaps wise to check that the other functions using check_type follow the same - i see @inheritParams being used for a lot of them.

[bms63]

Functions with check_type

[bundfussr]

Should we rename check_type and check_keys to check_unique?

[bms63]

Actually - that would make more sense to rename to check_unique.

[zdz2101]

I like this idea

[manciniedoardo]

I second @bundfussr 's idea, both parameters are checking uniqueness so this makes a lot of sense.

[bms63]

derive_param_tte is a good example where by_vars is different from other by_vars

[bms63]

Proposal for dataset_ref for derive_locf_records()

I'm okay with replaceing this with dataset_ref, but this is really a lookup table similar to derive_vars_merged_lookup(). We should be sure to specify in this issue.

Also, similar musing as by_vars do we want to unifiy the messaging for each one as they differ

[bms63]

@zdz2101 @ddsjoberg @bundfussr @manciniedoardo

apologies all - I originally had dataset_add , but this should be updated with dataset_ref, which is what @zdz2101 had originally proposed.

Reflecting a little more: perhaps derive_vars_merged_lookup() argument dataset_add should be changed to dataset_ref to match proposed update and keep in line with other functions that expect lookup tables or tables with all combinations of variables.

[manciniedoardo]

I would be in favour of always using dataset_ref for lookup table datasets which include all combinations of visits/params, and then unifying the description langauge. In the case above i agree both can become dataset_ref.

[bundfussr]

I would use dataset_ref for derive_locf_records() but not for derive_vars_merged_lookup(). So far we use dataset_ref to ensure that all groups (subjects, parameters, visits, ...) which are in dataset_ref are also included in the output dataset (even if they are not in the input datasets (dataset, dataset_add)).
In derive_vars_merged_lookup() only parameters which are included in the input dataset are included in the output dataset. If a parameter is in dataset_add but not in dataset, it is not in the output dataset. Thus I would not use dataset_ref.

Furthermore, we should not use dataset_ref for derive_var*() functions at all because these never add observations.

[zdz2101]

Proposal to swap out ellipses/args and pass in list of certain class e.g. params, dthcaus_source, date_source, records_source. into the function argument "sources" , discussed in re-work on 20JUL2023

[rossfarrugia]

agree with that - and its consistent with other functions like in {admiralonco} where we use reference_date: e.g. https://pharmaverse.github.io/admiralonco/main/reference/derive_param_bor.html

[zdz2101]

I am fine with a wording update, I originally paired this with start_date based on the description, but @rossfarrugia brings up a good point of the arguments in other packages (as I hadn't checked for the arguments there), if this flavor of reference_date is used elsewhere more reason to keep it as is.

[bundfussr]

I agree

[manciniedoardo]

I agree, as @bms63 suggested we could change "start" in the documentation to "reference".

[bms63]

traceability_vars

Looking into updating to set_values_to

[bms63]

There is a suitable difference between keep_source_vars and traceabiltiy_vars ... think set values to...

[bms63]

@zdz2101 can you put your summation here please!!!

[zdz2101]

A little two-parter here:

1. In general, can we replace traceability_vars instances with set_values_to ?
2. Might be off-base, but am I correct in undertstanding that the subtle difference here with traceability_vars vs keep_source_vars is that traceability_vars expects a named list where the values are more "flexible" in the sense that it can be retained + modified vs just simply retained? So for the example above, we are setting LALVDOM as the string "AE". Alternatively, we could have done something like LALVDOM = DOMAIN which assuming its coming from SDTM.AE, the column DOMAIN is likely all set to "AE". Versus keep_source_vars only selects and retains the variables from the source_dataset, so keep_source_vars = exprs(DOMAIN) would join + keep the DOMAIN and user can rename on their own as LALVDOM after using something like dplyr::rename()
   2b. I think we need to clarify use-cases if they are indeed different

[bundfussr]

1. Yes
2. Your analysis is correct. Maybe we should add examples to the description of the arguments in the programming strategy.

[bms63]

keep_source_vars

Proposal to add this argument to other admiral functions. Which ones need this added? Functions that uses source_datasets would possibly need this update and would be a good place to start on this adventure..

[bundfussr]

I think we should add it to all selection functions which add new observations.

[bms63]

Slack post created https://pharmaverse.slack.com/archives/C028SJ83KM1/p1692633891115359

[bms63]

Possible candidates for standardization of text in arguments

[bms63]

Look into set_values_to or other candidates??

[bms63]

derive_var_dthcaus(), derive_var_extreme_dt(), and derive_var_extreme_dtm()

All very similar functions - need to investigate if they can be consolidated.

[rossfarrugia]

I don't feel that we're in transition towards only ever having generic functions though either. we're probably more aiming for somewhere in the middle where we have some useful very generic functions that can do a lot but also its fine to have some targeted functions for specific common ADaM variable/parameters that will often be repeated and users value having a quick and easy way to derive them, e.g. DTHCAUS, BASE, ATOXGR, ONTRTFL, BMI, BSA to name just a few. nothing's changed in my mind for derive_var_dthcaus() - i still find it really useful and feel that taking it away would be at the detriment of our users. happy to hear any argument of those in favour of deprecating though.

[bundfussr]

I agree that we should not end up with generic functions only. Then we would have a major gap in the ADaM creation process. At the moment (and in the past) we replace specific function by calls to generic functions in the vignettes/templates. This has some advantages:

• less maintenance for testing and documentation for us
• less functions to learn for the user
• the derivations are clearer for the user

But also some disadvantages:

• Vignettes and templates are not well searchable
• They are not modular, e.g., you can share a complete template but not a single derivation or a subset of the derivations.

I would like to have an approach without these disadvantages.

I doubt that we would implement derive_var_dthcaus(), derive_var_extreme_dt(), and derive_var_extreme_dtm() today. For example you can use derive_var_dthcaus() instead of derive_var_extreme_dt() for deriving last known alive date. There are cases where you need to do this. If traceability variables are used and the date is not unique, an "arbitrary" observation is selected by derive_var_extreme_dt(). In derive_var_dthcaus() additional order variables can be specified to achieve uniqueness. But calling derive_var_dthcaus() to derive last known alive date is confusing. Therefore I would try to consolidate them. I would also ensure that the behavior and name of the consolidated function and its arguments is consistent with related functions, e.g., derive_extreme_event() is similar except that it derives new observations instead of new variable.

[rossfarrugia]

Has any user ever given this feedback? I get what you're saying that they do similar things under the hood, but when I wrote the ADSL template for Roche I found both functions very intuitive to use and I valued the very specific examples in each functional reference page that helped me to perform the common derivations DTHCAUS and LSTALVDT - as opposed to digging into a single more generic function that would take bit more work to customize for each specific purpose. I also find it easy to understand just by the function names that extreme_dt is first/last date and dthcaus is for picking out causes of death info.

On your example of people needing to use derive_var_dthcaus() instead of derive_var_extreme_dt(), wouldn't a possible solution be to make derive_var_extreme_dt() (and dtm) more consistent here by adding the same logic: If dates are equivalent, the first source will be kept, so the user should provide the inputs in the preferred order?

I leave it to @bms63 and @manciniedoardo to make the call here, but I worry that your logic above could be applied to so many other places in {admiral} to remove the more specific functions that make it easy for users and instead have them do a bit more work with a generic function. It's a fine balance between what's best for developers and users, and for me if ever unsure I'd side with not changing it - as every deprecation impacts our users - and I could be wrong but I don't see them asking for these changes.

[bundfussr]

We got feedback (almost from the beginning) that it is hard to find the function and function call for a particular variable/parameter. #2052 was created due to this feedback. Furthermore, also the opposite happens: users implement a new function although a (general) function with the required functionality exists. We observed this for example when reviewing admiralvaccine.
I think one of the reason for this issue is that we have both very general and very specific functions in our reference. Therefore I would separate them. The general functions should stay in the reference and the specific function calls are moved to a source which is easier to update and well searchable. Then users which need to implement a particular derivation could search the function call source first. If it does not include the derivation, they can look into the function reference to find a function which could be used. And ideally they store the new function call in the function call source.
Furthermore, in the function call source we could provide much more than we currently do via vignettes/templates and the few specific functions.

[manciniedoardo]

Apologies for the late reply here as I was away.

I discussed this with @bms63 and we favour inaction here, i.e. we want to stick with the past decision of not condensing the functions, even if they have similar purposes. This would minimise impact to end-users here and feel like if the functions are treating very common variables, there is limited risk in just keeping them both.

On a more general note, we'd like to straddle the line between having only generic functions and having very specific functions, because the latter will make {admiral}'s barrier to entry too high and the former doesn't leverage the abstractions of ADaM enough. In this specific case, we feel that keeping the functions as they are is a satisfactory compromise.

[bundfussr]

I got an interesting question from a user this morning. He wants to add a flag to ADSL which flags subjects which received both DRUG A and DRUG B within the first eight days. He tried

adsl_treat <- adsl_dcut %>%
  derive_var_merged_exist_flag(
    dataset_add = ex_ext,
    by_vars = exprs(STUDYID, USUBJID),
    new_var = drugs_flag,
    filter_add = EXDOSE > 0,
    condition = (str_detect(EXTRT, "DRUGA") & str_detect(EXTRT, "DRUGB") & (VISITDY < 8)), 
    true_value = "Y",
    false_value = "N"
  )

This does not produce the desired result because there is no record in ex_ext where EXTRT equals both "DRUGA" and "DRUGB". In principle we have the derive_*_joined*() functions for conditions considering more than one record. However, in derive_var_joined_exist_flag() you could specify filter_joined = any(str_detect(EXTRT, "DRUGA")) & any(str_detect(EXTRT, "DRUGB")) & VISITDY < 8 but it does not provide the dataset_add argument. derive_vars_joined() provides the dataset_add argument but it does not allow summary functions like any() in filter_joined.
An alternative to the *joined*() functions is allowing summary functions in the condition in derive_vars_merged(). Then condition = any(str_detect(EXTRT, "DRUGA")) & any(str_detect(EXTRT, "DRUGB")) & VISITDY < 8 could be used.

I would propose that we

• make the arguments and the behavior of the *joined*() functions consistent and
• make the behavior of arguments expecting conditions consistent with respect to using summary functions.

[bundfussr]

Should we add a description field to all "source" objects like for event() and event_joined() (see #1960)?

[bundfussr]

@rossfarrugia commented:

@bundfussr do you think users might get confused that we have event_sources.R that runs event() to create ADRS objects, and tte_sources.R that runs event_source() to create ADTTE objects. I just got myself in a tangle trying to understand the difference between event_sources and event_source, and then figured it out. I don't have any alternative naming solution to suggest either though 😕

I agree that it may confuse users and that the naming could be improved.

What do you think?

[rossfarrugia]

@bundfussr i think the potentially easiest solution here is in admiralonco call it response_sources.R instead of event_sources.R in pharmaverse/admiralonco#240. i think that makes it clear enough and more findable and means we don't need to change anything on admiral side. what do you think?

[bundfussr]

I think the .R files are not an issue because most users will never see them. The main issue is that we have event() and event_source(). From the names it is not clear what the difference is. In principle event() should have been named event_source() to be in line how we named such source objects before. However, this was not possible as the name was already been used.
I think we should review the names of the source objects to make them consistent and user-friendly.

[rossfarrugia]

fair point! i still think response_sources.R is a better filename even if just for helping us developers though. for the other point, how about extreme_source() instead of event()? Given this function is intended for use with derive_extreme_event(). would that be a quick fix that we could squeeze into this release to save people getting used to using event() that we know would likely change?

[bundfussr]

response_sources.R is a better filename

I agree. I'll rename it.

Regarding naming the source objects, I would not do a quick fix now. The risk is too high that we need to change it again. I would do a thorough review after the release. Hopefully this results in names which are consistent, user-friendly, and do not change in the next years.

[bundfussr]

Sometimes variables providing codes/ids like CMPNC ("Standardized Medication Name Code") are character variables. In the admiral queries dataset we use TERMID for numeric terms and TERMNAME for character terms. This led to some confusion in the case that CMPNC is the source variable. I would propose to rename TERMID and TERMNAME to TERMNUM and TERMCHAR respectively. Then the purpose of the variables is clearer.

[bundfussr]

I have uploaded a document (admiral Functions Overview) which hopefully supports the clean-up discussion. I tried to classify the functions and remove inconsistencies in the naming and behavior. I also tried to think a bit further: What else beyond admiral is required to produce ADaMs?
My idea is to consider admiral as whole.
Please have a look and add comments and ideas. I am sure I have missed a lot.

Please note that I focused on creating a consistent and user-friendly structure of admiral only. I did not take into account decisions which were made in the past (see thread above) because I am not sure which past decisions are discussible and which not.

@bms63 , when should we restart our clean-up discussions?

[bms63]

@zdz2101 Do you mind reviewing this thread for any missed actions and making them into an issue. If things are warranting more discussion, then lets tack on to the end of @bundfussr word document so fewer places to keep track of things.

[bms63]

@zdz2101 did we finish this task?

[zdz2101]

@bms63 In terms of migrating things that were here onto the doc, yes that task is done. As for turning outstanding things on the doc into github issues/posts for 2.0 I think we might need to review it together