Add picks variable selection to ae_oview module

[osenan]

Pull Request

This partially closes #330

I based my changes in this PR.

Please compare both modules with this example app (requires to have teal.picks installed, osprey)

library("teal")
library("teal.picks")
devtools::load_all()

data <- teal_data() %>%
  within({
    library(dplyr)
    ADSL <- teal.data::rADSL
    ADAE <- teal.data::rADAE
    .add_event_flags <- function(dat) {
      dat <- dat %>%
        mutate(
          TMPFL_SER = AESER == "Y",
          TMPFL_REL = AEREL == "Y",
          TMPFL_GR5 = AETOXGR == "5",
          AEREL1 = (AEREL == "Y" & ACTARM == "A: Drug X"),
          AEREL2 = (AEREL == "Y" & ACTARM == "B: Placebo")
        )
      labels <- c(
        "Serious AE", "Related AE", "Grade 5 AE",
        "AE related to A: Drug X", "AE related to B: Placebo"
      )
      cols <- c("TMPFL_SER", "TMPFL_REL", "TMPFL_GR5", "AEREL1", "AEREL2")
      for (i in seq_along(labels)) {
        attr(dat[[cols[i]]], "label") <- labels[i]
      }
      dat
    }
    ADAE <- .add_event_flags(ADAE)
  })

join_keys(data) <- default_cdisc_join_keys[names(data)]

app <- init(
  data = data,
  modules = modules(
    tm_g_ae_oview(
      label = "Common AE (picks)",
      flag_var_anl = teal.picks::picks(
        teal.picks::datasets("ADAE"),
        teal.picks::variables(
           choices = c("TMPFL_SER", "TMPFL_REL", "TMPFL_GR5", "AEREL1", "AEREL2"),
          selected = "TMPFL_SER"
        )
      ),
      arm_var = teal.picks::picks(
        teal.picks::datasets("ADSL"),
        teal.picks::variables(
          choices = teal.picks::is_categorical(min.len = 2),
          selected = "ACTARMCD"
        )
      ),
      plot_height = c(600, 200, 2000)
    ),
    # Default method (old) for comparison
    tm_g_ae_oview(
      label = "Common AE (default)",
      dataname = "ADAE",
      flag_var_anl = teal.transform::choices_selected(
        selected = "AEREL1",
        choices = teal.transform::variable_choices(
          data[["ADAE"]],
          c("TMPFL_SER", "TMPFL_REL", "TMPFL_GR5", "AEREL1", "AEREL2")
        )
      ),
      arm_var = teal.transform::choices_selected(
        selected = "ACTARMCD",
        choices = c("ACTARM", "ACTARMCD")
      ),
      plot_height = c(600, 200, 2000)
    )
  )
)

shinyApp(app$ui, app$server)

I refactored the tm_g_ae_oview module to create a generic, keep the default for teal.transform and add a method for picks variable selection. The module works as expected and setting the variables is better with picks. The issue reported in the similar PR on the plot dimensions is fixed if we set the plot dimensions in the module. The ui of the picks ui components is different and it breaks, in my opinion, the color pattern of the teal app.

Here the teal.transform module

I added simple tests for the module as well

[github-actions]

🎉 Thank you for your contribution! Before this PR can be accepted, we require that you read and agree to our Contributor License Agreement.
You can digitally sign the CLA by posting a comment on this Pull Request in the format shown below. This agreement will apply to this PR as well as all future contributions on this repository.

---

I have read the CLA Document and I hereby sign the CLA

---

osenan seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.}

[osenan]

I have read the CLA Document and I hereby sign the CLA

[osenan]

Changed to draft as I need to fix:

• Checks
• Plot
• Decorators

[averissimo]

Some preliminary comments.

• I believe this is a good case for only keeping 1 version of the module logic. (We can convert directly from choices_selected -> variables())
• I see that you're using picks(), consider using variables() instead for arm_var and flag_var_anl
  • And then create picks() inside the tm_g_ae_oview() call

[osenan]

I have read the CLA Document and I hereby sign the CLA

[osenan]

The plot and decorator I will handle in a separate issue finally

[github-actions]

Code Coverage Summary

Filename                         Stmts    Miss  Cover    Missing
-----------------------------  -------  ------  -------  -------------------------------------------------
R/tm_g_ae_oview.R                  271     210  22.51%   90-94, 98-102, 169-402
R/tm_g_ae_sub.R                    288     288  0.00%    60-379
R/tm_g_butterfly.R                 371     371  0.00%    125-534
R/tm_g_decorate.R                   42      42  0.00%    17-91
R/tm_g_events_term_id_picks.R      305     305  0.00%    23-406
R/tm_g_events_term_id.R            289     247  14.53%   131-407
R/tm_g_heat_bygrade.R              293     293  0.00%    136-459
R/tm_g_patient_profile.R           691     691  0.00%    159-913
R/tm_g_spiderplot.R                322     322  0.00%    100-464
R/tm_g_swimlane.R                  332     288  13.25%   65, 124-459
R/tm_g_waterfall.R                 414     374  9.66%    80-82, 141-569
R/utils_picks.R                     90      37  58.89%   18, 29-30, 55-76, 102, 106, 122-126, 146, 152-156
R/utils.R                           64      59  7.81%    31-73, 107-201
R/zzz.R                              3       3  0.00%    4-7
TOTAL                             3775    3530  6.49%

Diff against main

Filename                         Stmts    Miss  Cover
-----------------------------  -------  ------  --------
R/tm_g_ae_oview.R                  +46     -15  +22.51%
R/tm_g_events_term_id_picks.R     +305    +305  +100.00%
R/tm_g_events_term_id.R            +18     -24  +14.53%
R/tm_g_swimlane.R                  -13     -57  +13.25%
R/tm_g_waterfall.R                 -15     -55  +9.66%
R/utils_picks.R                    +90     +37  +58.89%
R/utils.R                          -21     -21  +1.93%
TOTAL                             +410    +170  +6.34%

Results for commit: e8572b5

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

[github-actions]

Unit Tests Summary

 1 files  ±0   5 suites  +1   0s ⏱️ ±0s
17 tests +4  12 ✅ +4  5 💤 ±0  0 ❌ ±0 
49 runs  +9  44 ✅ +9  5 💤 ±0  0 ❌ ±0 

Results for commit e8572b5. ± Comparison against base commit 80256d0.

♻️ This comment has been updated with latest results.

[averissimo]

Ready to be merged as it works as expected.

I have one last ask that both me and @m7pr have been doing in tmc and it also makes sense here.

Instead of S3 class we can just convert choices_selected in the body of the function, in essence support both without the overhead of S3.

Check this R/tm_t_pp_basic_info.R: https://github.com/insightsengineering/teal.modules.clinical/pull/1476/changes#diff-d50ab07375ab0f048cc866c3f6c0a0e7ed77c779483742bd3eebf05156f2a677

[averissimo]

It's almost ready! 👍

My main request is about the default arguments, in this package it might be better to leave them <missing>.

The second main comment here is about the implementation of the conversion. I would prefer to keep this only *if really needed* as it breaks with best practices of R and this project.

I understand the requirements for the util, but maybe we can find a middle ground, see my suggestion and we can then apply "upstrean" on tmc

[averissimo]

Awesome! Seems fine by me

[m7pr]

Do not prefix functions in examples nor vignettes.
Only prefix in tests and in the code/implemetnation

[m7pr]

When you move teal.picks in DESCRIPTION from Imports to Depends, you will not need to prefix those functions for the examples to work. As you didn't need to prefix teal.transform functions

[m7pr]

I know @averissimo suggested to drop default values, but we do have default values and the checks in tmc :P

[m7pr]

this one is not critical

[osenan]

In this case, the default was empty so I will keep it empty

[m7pr]

Is this possible to make it a wrapper, and reuse in all modules in this package? You could keep this wrapper in the main feature branch and reuse in single branches for specific modules.

[m7pr]

If possible use base R instead of rlang

[m7pr]

Also rlang was not in imports/depends so you get this warning in R CMD CHECK
https://github.com/insightsengineering/teal.osprey/actions/runs/24245023719/job/70788946164?pr=331#step:44:98

[m7pr]

we agreed to add testthat:: prefix for all tests for all functions besides it function, as it collideas with some namespace. check other packages in teal family to see that it is barely prefixed

[m7pr]

teal.transform is in the Depends, and teal.picks is a substitute, so should also be in the Depends. the same we did in tmc