admiral's use within GSK's ADaM metadata driven system

[bms63]

Why some existing admiral functions are not a good fit to be used in Executor
Thursday, February 22, 2024
1:33 PM
Executor is designed to control the inputs and outputs of each derivation, so that when selecting multiple derivations from the library the system can recognize the order of their execution automatically.

Information about inputs and outputs of each derivation is collected as metadata from the user and in order to ensure consistency between what is specified in the metadata and what is actually being used in the derivation the frame work performs the initial filtering and joining of the source data as well as joining the result on to output dataset itself, taking this actions off the user's table.

This makes functions like the following:

and other functions utilizing derive_vars_merged function under the hood not useful with Executor.

Proposal:
For each function using derive_vars_merged abstract the part before the derive_vars_merged call into a separate function and call is from the main function.
e.g.

derive_var_merged_exist_flag <- function(dataset,
                                         dataset_add,
                                         by_vars,
                                         new_var,
                                         condition,
                                         true_value = "Y",
                                         false_value = NA_character_,
                                         missing_value = NA_character_,
                                         filter_add = NULL) {
  new_var <- assert_symbol(enexpr(new_var))
  condition <- assert_filter_cond(enexpr(condition))
  filter_add <-
    assert_filter_cond(enexpr(filter_add), optional = TRUE)
 
  add_data <- filter_if(dataset_add, filter_add) %>%
    mutate(!!new_var := if_else(!!condition, 1, 0, 0))
 
  derive_vars_merged(
    dataset,
    dataset_add = add_data,
    by_vars = by_vars,
    new_vars = exprs(!!new_var),
    order = exprs(!!new_var),
    check_type = "none",
    mode = "last"
  ) %>%
    mutate(!!new_var := if_else(!!new_var == 1, true_value, false_value, missing_value))
}

Turned into:

exist_flag <- function(df,
                                         new_var,
                                         condition,
                                         true_value = "Y",
                                         false_value = NA_character_,
                                         missing_value = NA_character_,
                                         filter_add = NULL
                                         ) {

  new_var <- assert_symbol(enexpr(new_var))
  condition <- assert_filter_cond(enexpr(condition))  
  filter_add <-
    assert_filter_cond(enexpr(filter_add), optional = TRUE)
 
  add_data <- filter_if(dataset_add, filter_add) %>%
    mutate(!!new_var := if_else(!!condition, 1, 0, 0))
}
 
derive_var_merged_exist_flag <- function(dataset,
                                         dataset_add,
                                         by_vars,
                                         new_var,
                                         condition,
                                         true_value = "Y",
                                         false_value = NA_character_,
                                         missing_value = NA_character_,
                                         filter_add = NULL) {

  add_data <- exist_flag(df, new_var, condition, true_value, false_value, missing_value, filter_add)
 
  derive_vars_merged(
    dataset,
    dataset_add = add_data,
    by_vars = by_vars,
    new_vars = exprs(!!new_var),
    order = exprs(!!new_var),
    check_type = "none",
    mode = "last"
  ) %>%
    mutate(!!new_var := if_else(!!new_var == 1, true_value, false_value, missing_value))
}

This will allow users, who are not keen on directly using merging onto output dataset make use of admiral functionality without maintaining a separate fork of the repo.

The proposal is that:
• Admiral or GSK Mercury team implements the proposed change and raises as PR in admiral repository
• Admiral team prioritizes the review of the PR and release to make sure Mercury can meet our timelines (release end of Q2 latest)
• Implementing abstracting functions like exist_flag becomes standard practice in admiral

Alexey

[bms63]

@bundfussr @rossfarrugia @manciniedoardo I am meeting with Alexey ( @paltusplintus ) on Monday to discuss more. But wanted to bring this to your attention about some possible requests/needs coming from GSK.

@paltusplintus anything else you would like to add?

@kaz462 I know Bayer has a metadata system being developed. Do you all have any requirements?

[rossfarrugia]

OK thanks Ben, keep us updated. my initial thoughts are this will create a lot more functions in our package and it adds another layer of abstracting away code which isn't great for users and the simplicity we strive for in our design principles, and it also sounds quite a big refactor so sounds more like a 2.0.0 to me. would have been good to have this input earlier to feed into the admiral design. would of course be preferable at this stage if any way for GSK to use admiral as it is and make updates on the side of the internal tools. we have pieces around metadata at Roche but we deal with those on the admiralroche side so as not to impose company-specific approaches on the open source industry-wide packages. let's chat more on this at the next sponsors/lead meeting after PHUSE.

[bms63]

Thanks for the reply @rossfarrugia!!

I'll give some report back after the meeting on Monday. A big refactor that doesn't impact other users and companies seems highly unlikely/impossible to pull off. However, perhaps some tweaks and minor changes could be made/suggested to help build an admiralgsk that to help with GSK's internal tools over the coming release cycles.

Be great to get some others from GSK into the sponsor/lead meeting to discuss more on direction and support for the long haul of admiral. Especially if there is an admiralgsk with an admiral dependency!!

[manciniedoardo]

After an initial read, I am very much partial to protecting users/other companies from too much impact by incorporating some of these changes in an {admiralgsk} rather than directly within {admiral}. Ideologically I see {admiral} as being the lowest common denominator in for ADaM creation in R across pharma, whereas with {admiral<company>{} sponsors are then free to build upon {admiral} and potentially layer complexity to suit their specific needs.

Nevertheless, this would have to be discussed in more depth as it's not even immediately clear how to integrate the above in a company extension package. One option could be to slightly modify derive_vars_merged_exist_flag() and similar functions 9I see from @bundfussr 's comment below that there may not even be that many) by adding in a toggleable option to trigger the requested behaviour, though this already veers away from the usual {admiral} behaviour and may confuse users who accidentally discover it.

In any case I am all for ensuring {admiral} can be built upon, and I am sure we can work something out 😄

[bms63]

Adding @SteveWLaw from GSK to discussion.

[bundfussr]

I had a short look at admiral and tried to identify the affected functions. It seems that only two functions need to be updated: derive_var_merged_exist_flag() and derive_vars_query(). For the other functions which are calling derive_vars_merged() (derive_vars_merged_summary(), derive_vars_extreme_event(), derive_vars_computed()) the part before the merge is already done by calling an admiral function, e.g., in derive_vars_merged_summary() derive_summary_records() is called.

Does the final join step in your tool include selecting the first or last observation?

[paltusplintus]

Thanks Stefan! It seems that our ask is basically to follow the approach implemented in derive_vars_merged_summary(), derive_vars_extreme_event(), derive_vars_computed() and refactor derive_var_merged_exist_flag() and derive_vars_query() to follow it (which should not impact in any way the users of those 2 functions). We are happy to create a PR with the proposed update for those 2, but need to make sure admiral team is in agreement to follow the approach going forward to avoid surprises with our metadata-driven system.
At the moment I don't see how we could future-proof implement it in admiralgsk without huge maintenance overhead in the future and prefer admiral to be the lowest common denominator as Edoardo mentions.

Also, given that we are in the beginning of implementing metadata-driven admiral use, you might hear back from us again with further suggestions/feedback.

[rossfarrugia]

@paltusplintus it doesn't sound too major an impact for this one so makes sense if your team could put together a PR for the admiral team to consider. would be easier to then visualize and review impact rather than in a discussion thread.

one request would be if there could be a more holistic review of the admiral packages with your GSK system so that we get an idea of other potential requests, as we want to be cautious that if we were to accept this PR then this doesn't open the door for any more major refactors later.

all in all though, great that GSK are looking seriously into embedding usage of admiral. as one of the 2 co-founder companies it would be a bad look if they weren't 🤣

[kaz462]

@kaz462 I know Bayer has a metadata system being developed. Do you all have any requirements?

thx @bms63 , we have a metadata structure that connects with our internal SAS macros, which I believe could also be adapted for R functions. If the following two issues can be implemented in metacore/metatools, it could align with Bayer metadata and reduce some work at admiralbayer:
atorus-research/metacore#99
pharmaverse/metatools#56

It seems there are already some implementations for metadata-driven solutions from Roche/GSK. Would it still be of interest to harmonize ADaM metadata through metacore and potentially build a metadata-driven system together?
@daniel-woodie from Eli Lilly was also looking into this piece (pharmaverse/metatools#55) and perhaps doing something similar?

[bms63]

I am closing this discussion as GSK's requests are being implemented here - #2425 (comment)