Add picks variable selection to ae_oview module by osenan · Pull Request #331 · insightsengineering/teal.osprey

osenan · 2026-03-12T11:37:38Z

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 · 2026-03-12T11:37:58Z

🎉 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 · 2026-03-12T13:34:57Z

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

osenan · 2026-03-12T15:00:17Z

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

Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com> Signed-off-by: Oriol Senan <35930244+osenan@users.noreply.github.com>

osenan · 2026-03-16T11:42:32Z

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

osenan · 2026-03-17T06:46:27Z

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

github-actions · 2026-03-17T06:55:35Z

Code Coverage Summary

Filename                    Stmts    Miss  Cover    Missing
------------------------  -------  ------  -------  -----------------------
R/tm_g_ae_oview.R             274     210  23.36%   93-97, 101-105, 172-405
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.R       271     271  0.00%    63-364
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             345     345  0.00%    127-525
R/tm_g_waterfall.R            429     429  0.00%    109-590
R/utils.R                      91      80  12.09%   31-73, 107-240
R/zzz.R                         3       3  0.00%    4-7
TOTAL                        3420    3345  2.19%

Diff against main

Filename             Stmts    Miss  Cover
-----------------  -------  ------  -------
R/tm_g_ae_oview.R      +49     -15  +23.36%
R/utils.R               +6       0  +6.21%
TOTAL                  +55     -15  +2.04%

Results for commit: bce4924

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

github-actions · 2026-03-17T06:55:42Z

Unit Tests Summary

1 files 2 suites 0s ⏱️
8 tests 8 ✅ 0 💤 0 ❌
20 runs 20 ✅ 0 💤 0 ❌

Results for commit bce4924.

♻️ This comment has been updated with latest results.

averissimo

Awesome! It works great and it even supports picks conversion for choices_selected

Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com> Signed-off-by: Oriol Senan <35930244+osenan@users.noreply.github.com>

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

Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com> Signed-off-by: Oriol Senan <35930244+osenan@users.noreply.github.com>

averissimo

Awesome! Seems fine by me

Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com> Signed-off-by: Oriol Senan <35930244+osenan@users.noreply.github.com>

m7pr · 2026-04-13T09:14:10Z

+#'       arm_var = teal.picks::variables(
+#'         choices = dplyr::starts_with("ACTARM"),


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

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 · 2026-04-13T09:14:41Z

+  arm_var,
+  flag_var_anl,


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

this one is not critical

m7pr · 2026-04-13T09:15:13Z

+  if (isTRUE(attr(arm_var$variables, "multiple"))) {
+    warning(
+      "`arm_var` accepts only a single variable selection. ",
+      "Forcing `teal.picks::variables(multiple)` to FALSE."
+    )
+    attr(arm_var$variables, "multiple") <- FALSE
+  }
+
+  if (isTRUE(attr(flag_var_anl$variables, "multiple"))) {
+    warning(
+      "`flag_var_anl` accepts only a single variable selection. ",
+      "Forcing `teal.picks::variables(multiple)` to FALSE."
+    )
+    attr(flag_var_anl$variables, "multiple") <- FALSE
+  }


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 · 2026-04-13T09:16:29Z

+  arg <- rlang::ensym(arg)
+  arg_name <- as.character(arg)
+  arg <- rlang::eval_tidy(arg, env = envir)


If possible use base R instead of rlang

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 · 2026-04-13T09:17:19Z

+    )
+  })
+
+  testthat::it("fails when flag_var_anl is neiter picks or choices_selected", {


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 · 2026-04-13T09:18:02Z

    teal.code (>= 0.7.0),
    teal.data (>= 0.8.0),
    teal.logger (>= 0.4.0),
+    teal.picks (>= 0.1.0),


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

osenan and others added 4 commits March 12, 2026 12:14

feat: add S3 method dispatch for picks or teal transform variables

060314d

tests: add test for tm_g_ae_oview module

975ae0a

chore: fix spelling

b6fd8f6

chore: fix sign credentials

d68f94a

osenan added the core label Mar 12, 2026

osenan requested a review from a team March 12, 2026 11:38

[skip style] [skip vbump] Restyle files

3dd972e

osenan mentioned this pull request Mar 12, 2026

[Feature Request]: Allow modules to use teal.picks #330

Open

3 tasks

osenan marked this pull request as draft March 12, 2026 14:59

averissimo reviewed Mar 12, 2026

View reviewed changes

averissimo mentioned this pull request Mar 13, 2026

[Feature Request]: Implement picks on modules insightsengineering/teal.modules.clinical#1472

Open

osenan and others added 4 commits March 16, 2026 12:31

Update DESCRIPTION

89ecbea

Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com> Signed-off-by: Oriol Senan <35930244+osenan@users.noreply.github.com>

Update R/tm_g_ae_oview_picks.R

630f5bf

Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com> Signed-off-by: Oriol Senan <35930244+osenan@users.noreply.github.com>

Update R/tm_g_ae_oview_picks.R

2869ce9

Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com> Signed-off-by: Oriol Senan <35930244+osenan@users.noreply.github.com>

Update R/tm_g_ae_oview_picks.R

c2f07a3

Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com> Signed-off-by: Oriol Senan <35930244+osenan@users.noreply.github.com>

osenan added 5 commits March 16, 2026 12:53

chore: update docs after fix style

bf70fff

chore: fix typo in description

88fbf7e

refactor: change defaults on ae_oview generic and use dataname directly

8bd9bd3

refactor: add only necessary arguments to the UI

584c88b

fix: update test after refactor on method

b3f3eb0

osenan marked this pull request as ready for review March 17, 2026 06:45

github-actions Bot added 2 commits March 17, 2026 06:46

[skip style] [skip vbump] Restyle files

e00faee

[skip roxygen] [skip vbump] Roxygen Man Pages Auto Update

440b9fe

github-actions Bot and others added 4 commits March 17, 2026 10:15

[skip style] [skip vbump] Restyle files

df0f269

chore: fix R CMD check and lintr

b7219f0

[skip style] [skip vbump] Restyle files

640f80e

chore: fix styler and linter

6e6e222

osenan requested a review from averissimo March 17, 2026 11:56

averissimo requested changes Mar 17, 2026

View reviewed changes

Comment thread R/tm_g_ae_oview.R Outdated

Comment thread R/tm_g_ae_oview.R Outdated

Comment thread R/tm_g_ae_oview.R

averissimo self-assigned this Mar 17, 2026

osenan and others added 6 commits March 17, 2026 15:31

chore: revert to magrittr pipe

73c6f60

chore: remove pipe consistency linter as we need magrittr pipes

8ca1bc1

Update R/tm_g_ae_oview.R

0e4d1bc

Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com> Signed-off-by: Oriol Senan <35930244+osenan@users.noreply.github.com>

refactor: use as.picks to reuse picks srv and ui in choices_selected

af68495

chore: fix lintr comment to avoid automatic restyling

e5e9dad

Update R/tm_g_ae_oview_picks.R

5f0cfc2

Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com> Signed-off-by: Oriol Senan <35930244+osenan@users.noreply.github.com>

osenan requested a review from averissimo March 18, 2026 11:33

osenan mentioned this pull request Mar 20, 2026

Add picks to tm_g_butterfly module #335

Open

averissimo requested changes Mar 24, 2026

View reviewed changes

Comment thread R/tm_g_ae_oview.R Outdated

Comment thread tests/testthat/test-tm_g_ae_oview.R

osenan added 3 commits April 8, 2026 07:54

refactor: simplify and avoid S3 method dispatch

0c0b079

chore: remove references to unused methods

b91a4f7

chore: fix lintrs from CI

756d9a4

osenan requested a review from averissimo April 8, 2026 09:25

averissimo requested changes Apr 9, 2026

View reviewed changes

Comment thread R/tm_g_ae_oview.R Outdated

Comment thread R/utils.R Outdated

Comment thread R/tm_g_ae_oview.R Outdated

Comment thread R/tm_g_ae_oview.R

Comment thread R/utils.R Outdated

Comment thread R/tm_g_ae_oview.R Outdated

osenan and others added 2 commits April 9, 2026 16:44

refactor: avoid using parent envirionment and simplify conversion

ea0558b

Update R/tm_g_ae_oview.R

1678829

Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com> Signed-off-by: Oriol Senan <35930244+osenan@users.noreply.github.com>

averissimo approved these changes Apr 10, 2026

View reviewed changes

averissimo reviewed Apr 10, 2026

View reviewed changes

Comment thread R/utils.R Outdated

osenan and others added 2 commits April 10, 2026 15:17

Update R/utils.R

af27484

Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com> Signed-off-by: Oriol Senan <35930244+osenan@users.noreply.github.com>

[skip roxygen] [skip vbump] Roxygen Man Pages Auto Update

bce4924

osenan mentioned this pull request Apr 12, 2026

feat(picks): migrate tm_t_abnormality_by_worst_grade insightsengineering/teal.modules.clinical#1488

Merged

m7pr requested changes Apr 13, 2026

View reviewed changes

m7pr self-assigned this Apr 14, 2026

		#' arm_var = teal.picks::variables(
		#' choices = dplyr::starts_with("ACTARM"),

Uh oh!

Conversation

osenan commented Mar 12, 2026

Pull Request

Uh oh!

github-actions Bot commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

osenan commented Mar 12, 2026

Uh oh!

osenan commented Mar 12, 2026

Uh oh!

averissimo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

osenan commented Mar 16, 2026

Uh oh!

osenan commented Mar 17, 2026

Uh oh!

github-actions Bot commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage Summary

Diff against main

Uh oh!

github-actions Bot commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Tests Summary

Uh oh!

averissimo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

averissimo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

averissimo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

averissimo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

github-actions Bot commented Mar 12, 2026 •

edited

Loading

averissimo left a comment •

edited

Loading

github-actions Bot commented Mar 17, 2026 •

edited

Loading

github-actions Bot commented Mar 17, 2026 •

edited

Loading