Feature Request: Enable deriving more than one variable by derive_param_computed()

[bundfussr]

Feature Idea

derive_param_computed() should be enhanced such that more than one variable can be computed. The analysis_var and analysis_value argument should be removed and values of the variables should be specified in the set_values_to argument.

Relevant Input

No response

Relevant Output

No response

Reproducible Example/Pseudo Code

derive_param_computed(
  dataset_add = adlb_tbilialk,
  by_vars = exprs(STUDYID, USUBJID, TEMP_BL, TEMP_BLN),
  parameters = c("ALK2", "TBILI2"),
  set_values_to = exprs(
    AVALC = if_else(AVALC.TBILI2 == "Y" & AVALC.ALK2 == "Y", "Y", "N"),
    ADTM = pmax(ADTM.TBILI2, ADTM.ALK2),
    ADTF = if_else(ADTM == ADTM.TBILI2, ADTF.TBILI2, ADTF.ALK2),
    AVAL = yn_to_numeric(AVALC),
    PARAMCD = "TB2AK5",
    PARAM = "TBILI > 2 times ULN and ALKPH <= 5 times ULN"
  )
)

[bundfussr]

@pharmaverse/admiral , any objections to this enhancement?

[manciniedoardo]

@pharmaverse/admiral , any objections to this enhancement?

I like it. It's a breaking change, but it does add some very useful functionality. How would you have programmed this before?

[kaz462]

I like this enhancement! What do you think of enhancing set_values_to so that it can cover analysis_value or new_vars -

• enhance analysis_var to allow more variables
• enhance set_values_to to allow XX.XX
• remove analysis_value, or should we keep it to avoid breaking change?

derive_param_computed(
  dataset_add = adlb_tbilialk,
  by_vars = exprs(STUDYID, USUBJID, TEMP_BL, TEMP_BLN),
  parameters = c("ALK2", "TBILI2"),
  analysis_var =exprs(AVALC, ADTM, ADTF), ### enhance analysis_var to allow more variables
  set_values_to = exprs(
### enhance set_values_to allow XX.XX 
    AVALC = if_else(AVALC.TBILI2 == "Y" & AVALC.ALK2 == "Y", "Y", "N"),
    ADTM = pmax(ADTM.TBILI2, ADTM.ALK2),
    ADTF = if_else(ADTM == ADTM.TBILI2, ADTF.TBILI2, ADTF.ALK2),
### same as before    
    AVAL = yn_to_numeric(AVALC),
    PARAMCD = "TB2AK5",
    PARAM = "TBILI > 2 times ULN and ALKPH <= 5 times ULN",
  )
)

[manciniedoardo]

I like that idea @kaz462, however from a user perspective is this in line with how the rest of our package functions?

[bms63]

Apologies, I'm not onboard with this change.

Could someone just make a custom function to help them do this? How many Param/cd derivations are usually derived in an ADaM - 2-4? Smooshing them all into one function seems to reduce readability.

Also, I'm hesitant to break this function as I really like how clean and easy to use this function is already and rather focus on cleaning up other functions.

Perhaps a derive_multi_params_computed be created?

[bundfussr]

Apologies, I'm not onboard with this change.

Could someone just make a custom function to help them do this? How many Param/cd derivations are usually derived in an ADaM - 2-4? Smooshing them all into one function seems to reduce readability.

Also, I'm hesitant to break this function as I really like how clean and easy to use this function is already and rather focus on cleaning up other functions.

Perhaps a derive_multi_params_computed be created?

@bms63 , it is not intended to derive more than one parameter but more than one variable for a single parameter like in the example.

[bms63]

Ah think I have confused myself about this proposed update. Apologies.

As a constant user of this function I can see this update adding some nice value, but I am still wondering can we just have a more "advanced function" if someone wants this ability (which I think they will!!) and not break this function?

A derive_params_computed_ext - ext being extended. This way we can have separate documentation to showcase how to use the function with more advanced uses cases. The documentation/examples for derive_param_computed is already pretty long.

[manciniedoardo]

@bms63 I think this is a good compromise. Maybe let's go with that for now and re-evaluate in the future?

[bundfussr]

I like this enhancement! What do you think of enhancing set_values_to so that it can cover analysis_value or new_vars -

• enhance analysis_var to allow more variables
• enhance set_values_to to allow XX.XX
• remove analysis_value, or should we keep it to avoid breaking change?

derive_param_computed(
  dataset_add = adlb_tbilialk,
  by_vars = exprs(STUDYID, USUBJID, TEMP_BL, TEMP_BLN),
  parameters = c("ALK2", "TBILI2"),
  analysis_var =exprs(AVALC, ADTM, ADTF), ### enhance analysis_var to allow more variables
  set_values_to = exprs(
### enhance set_values_to allow XX.XX 
    AVALC = if_else(AVALC.TBILI2 == "Y" & AVALC.ALK2 == "Y", "Y", "N"),
    ADTM = pmax(ADTM.TBILI2, ADTM.ALK2),
    ADTF = if_else(ADTM == ADTM.TBILI2, ADTF.TBILI2, ADTF.ALK2),
### same as before    
    AVAL = yn_to_numeric(AVALC),
    PARAMCD = "TB2AK5",
    PARAM = "TBILI > 2 times ULN and ALKPH <= 5 times ULN",
  )
)

Good point!

Maybe we should use the new_vars argument only for the derive_var* functions and set_values_to for functions which add records. You can add new variables via set_values_to but usually you just set existing variables for the new observations. So this rule seems intuitive to me.

Do we need the analysis_var argument if we extend set_values_to as proposed?

[bundfussr]

Ah think I have confused myself about this proposed update. Apologies.

As a constant user of this function I can see this update adding some nice value, but I am still wondering can we just have a more "advanced function" if someone wants this ability (which I think they will!!) and not break this function?

A derive_params_computed_ext - ext being extended. This way we can have separate documentation to showcase how to use the function with more advanced uses cases. The documentation/examples for derive_param_computed is already pretty long.

I would not add a new function like derive_param_computed_ext(). The new function would cover derive_param_computed() completely and would have even less parameters than the existing function if we implement Kangjie's suggestion. So I see no benefit for users or developers. Furthermore, it seems to contradict our clean-up attempts. So far we tried to identify and remove overlapping functionality. Introducing derive_*_ext() function looks like a step in the opposite direction.

The example section is a general issue in R. At the moment there are three examples for derive_param_computed(). This seems not to many to me. I think the issue is that all examples are put into a single code block. I hope that the new roclet allows a better structure like title and description for each example and entries in the TOC.

[bms63]

Thanks @bundfussr - sorry that I am kicking and screaming into our glorious future :). At every release I am updating a lot of admiral code for my study so I am little sensitive to changes.

If there is a decrease in the documentation to understand how to use the function, then I can see that as a nice trade-off with breaking the function for the next release.

[manciniedoardo]

I like this enhancement! What do you think of enhancing set_values_to so that it can cover analysis_value or new_vars -

• enhance analysis_var to allow more variables
• enhance set_values_to to allow XX.XX
• remove analysis_value, or should we keep it to avoid breaking change?

derive_param_computed(
  dataset_add = adlb_tbilialk,
  by_vars = exprs(STUDYID, USUBJID, TEMP_BL, TEMP_BLN),
  parameters = c("ALK2", "TBILI2"),
  analysis_var =exprs(AVALC, ADTM, ADTF), ### enhance analysis_var to allow more variables
  set_values_to = exprs(
### enhance set_values_to allow XX.XX 
    AVALC = if_else(AVALC.TBILI2 == "Y" & AVALC.ALK2 == "Y", "Y", "N"),
    ADTM = pmax(ADTM.TBILI2, ADTM.ALK2),
    ADTF = if_else(ADTM == ADTM.TBILI2, ADTF.TBILI2, ADTF.ALK2),
### same as before    
    AVAL = yn_to_numeric(AVALC),
    PARAMCD = "TB2AK5",
    PARAM = "TBILI > 2 times ULN and ALKPH <= 5 times ULN",
  )
)

Good point!

Maybe we should use the new_vars argument only for the derive_var* functions and set_values_to for functions which add records. You can add new variables via set_values_to but usually you just set existing variables for the new observations. So this rule seems intuitive to me.

Do we need the analysis_var argument if we extend set_values_to as proposed?

Ok, I see @bundfussr 's point about contradiction of cleaning attempts with a new function.

Re-evaluating the suggestions given, it seems like @kaz462 's one above makes the most sense and is the most intuitive, but I too think we should remove analysis_var as it seems superfluous here.

Happy to go with this is if everyone is as well.

[bundfussr]

At every release I am updating a lot of admiral code for my study so I am little sensitive to changes.

@bms63 , I agree that all the changes are a burden for studies. It was a conscious decision to develop admiral in a way which requires a lot of updates over a long period of time. The following picture from one of the first presentations visualizes this strategy.

Unfortunately, I see no way changing this soon.

[manciniedoardo]

@bundfussr I love the picture. I do think that where possible we as developers should absorb the brunt of the changes to minimise effect on end-users though. Now and in the next quarters is a key time in which many users will start using the package so we don't want to lose them at the get-go. Even if it is at the expense of us making our life a bit harder.

[bundfussr]

How should we "absorb the brunt of the changes"???

[manciniedoardo]

A vague example:

We (admiral team) come up with an improvement to admiral functionality, which if implemented directly would improve both our lives as developers and also the reach of the package. But, it would be a breaking change, so would cause users who already have local admiral scripts to either not be able to upversion, or have to go through the code and make many changes. So, instead of the implementing the change directly, we implement more complex backend dual functionality so that the package is backwards compatible. Thus we absorb the brunt of the change to minimise impact on users.

A more specific example:

We just implemented a (much needed) breaking change switching from vars() to exprs(). We are now discussing whether to use tidyverse select instead. At some point in the future, even if we agree to switch to tidyverse select, that switch should be backwards compatible, at least at the beginning (ie, we add functionality to accept both types of expressions).

If we don't work with this mindset we risk losing users and becoming the only enjoyers of our own package!

[bundfussr]

A vague example:

We (admiral team) come up with an improvement to admiral functionality, which if implemented directly would improve both our lives as developers and also the reach of the package. But, it would be a breaking change, so would cause users who already have local admiral scripts to either not be able to upversion, or have to go through the code and make many changes. So, instead of the implementing the change directly, we implement more complex backend dual functionality so that the package is backwards compatible. Thus we absorb the brunt of the change to minimise impact on users.

A more specific example:

We just implemented a (much needed) breaking change switching from vars() to exprs(). We are now discussing whether to use tidyverse select instead. At some point in the future, even if we agree to switch to tidyverse select, that switch should be backwards compatible, at least at the beginning (ie, we add functionality to accept both types of expressions).

If we don't work with this mindset we risk losing users and becoming the only enjoyers of our own package!

@manciniedoardo , I see. However, I think that's exactly what our deprecation strategy ensures. Old code still works but the users are informed that a better alternative is available.

Do you think we need to change the deprecation strategy? For example, issue just a note in the first release cycle, a warning in the second, and an error in the third?

[bms63]

A vague example:
We (admiral team) come up with an improvement to admiral functionality, which if implemented directly would improve both our lives as developers and also the reach of the package. But, it would be a breaking change, so would cause users who already have local admiral scripts to either not be able to upversion, or have to go through the code and make many changes. So, instead of the implementing the change directly, we implement more complex backend dual functionality so that the package is backwards compatible. Thus we absorb the brunt of the change to minimise impact on users.
A more specific example:
We just implemented a (much needed) breaking change switching from vars() to exprs(). We are now discussing whether to use tidyverse select instead. At some point in the future, even if we agree to switch to tidyverse select, that switch should be backwards compatible, at least at the beginning (ie, we add functionality to accept both types of expressions).
If we don't work with this mindset we risk losing users and becoming the only enjoyers of our own package!

@manciniedoardo , I see. However, I think that's exactly what our deprecation strategy ensures. Old code still works but the users are informed that a better alternative is available.

Do you think we need to change the deprecation strategy? For example, issue just a note in the first release cycle, a warning in the second, and an error in the third?

Perhaps when we reach 1.0 we should use the note options to tell users we are planning on deprecating this in the future (multi-year deprecation process), but for now I think we should drop things as fast as possible until we get to 1.0

[manciniedoardo]

@bms63 @bundfussr We need to strike the right balance between changing fast ahead of v1.0 and not alienating users that are already on the platform. After all, at least on the Roche side we are likely to get a large influx of users before the end of the year, which will be a huge plus for us as a whole due to increased engagement and relevance for the package. But these new users won't really care whether we are on v1.0 or not, as they will need to use admiral for production regardless.

In any case, at least in concomitance with v1.0 we should slow down the deprecation strategy to something along the lines of what @bundfussr discussed.