datanames slot in teal_transform_module (ignore @datanames)

[gogonzo]

Alternative solution to #1327

#' @param datanames (`character`) A vector with `datanames` that are relevant for the item. The
#'   filter panel will automatically update the shown filters to include only
#'   filters in the listed datasets. `NULL` will hide the filter panel,
#'   and the keyword `"all"` will show filters of all datasets. `datanames` also determines
#'   a subset of datasets which are appended to the `data` argument in server function.

If you look at the ?module documentation of datanames argument, there is nothing about "if datanames is 'all' then all datasets from @datanames will be taken". It is very hard to explain as datanames is not only a module property, which makes it difficult to describe in a documentation. However, in the vignette we have it described in details see here

Here we have a following rules:

1. Module's server get's always module$datanames if specified.
   • When transform argument is specified in the module(transform) then module appends transform$datanames to module$datanames to always keep module$datanames as a single point of truth. transform is always added as an argument of module() function, so resolving datanames there makes sense and is the most optimal.
2. When module$datanames = "all" then all objects from transformed data is passed to the module.
   • When data contains datasets which shouldn't be displayed for a given module then one should module |> set_datanames() to limit displayed datasets

Advantage:

• single point of truth (module$datanames)
• don't need to take care of data@datanames
• can re-set datanames for the whole group of modules if needed

Disadvantages:

• If re-set is applied for all modules modules(mod1, mod2, mod3) |> set_datanames() then each module's datanames will be re-set even if their datanames have been set to what is exactly needed. Possible solution is to append_datanames() instead of set_datanames(), WDYT?

app

pkgload::load_all("teal")

trans <- list(
 anl_w_datanames = teal_transform_module(
   label = "ANL with datanames",
   ui = function(id) {
     ns <- NS(id)
     tagList(
       div("This transformer adds ANL based on specified ADSL, ADTTE"),
       numericInput(ns("obs"), "Number of subjects", value = 400, min = 0, max = 400)
     )
   },
   server = function(id, data) {
     moduleServer(id, function(input, output, session) {
       reactive({
         within(data(),
           {
             ANL <- dplyr::inner_join(
               head(ADSL, nobs),
               ADTTE[c("STUDYID", "USUBJID", setdiff(colnames(ADTTE), colnames(ADSL)))],
               by = c("USUBJID", "STUDYID")
             )
           },
           nobs = input$obs
         )
       })
     })
   },
   datanames = "ADTTE"
 ),
 anl_wout_datanames = teal_transform_module(
   label = "ANL without datanames",
   ui = function(id) {
     ns <- NS(id)
     tagList(
       div("This transformer adds ANL based on unspecified ADSL, ADTTE"),
       numericInput(ns("obs"), "Number of subjects", value = 400, min = 0, max = 400)
     )
   },
   server = function(id, data) {
     moduleServer(id, function(input, output, session) {
       reactive({
         within(data(),
           {
             ANL <- dplyr::inner_join(
               head(ADSL, nobs),
               ADTTE[c("STUDYID", "USUBJID", setdiff(colnames(ADTTE), colnames(ADSL)))],
               by = c("USUBJID", "STUDYID")
             )
           },
           nobs = input$obs
         )
       })
     })
   }
 ),
 adsl_w_datanames = teal_transform_module(
   label = "modify ADSL",
   ui = function(id) {
     ns <- NS(id)
     tagList(
       div("This transformer modifies ADSL based on specified ADTTE")
     )
   },
   server = function(id, data) {
     moduleServer(id, function(input, output, session) {
       reactive({
         within(data(),
           {
             ADTTE_summary <- ADTTE |>
               dplyr::group_by(STUDYID, USUBJID) |>
               dplyr::summarize(PARAMCD_AVAL = paste(paste(PARAMCD, "-", AVAL), collapse = "; "))
             ADSL <- dplyr::left_join(ADSL, ADTTE_summary)
           },
           nobs = input$obs
         )
       })
     })
   },
   datanames = c("ADSL", "ADTTE")
 ),
 adsl_wout_datanames = teal_transform_module(
   label = "modify ADSL",
   ui = function(id) {
     ns <- NS(id)
     tagList(
       div("This transformer modifies ADSL based on unspecified ADTTE")
     )
   },
   server = function(id, data) {
     moduleServer(id, function(input, output, session) {
       reactive({
         within(data(),
           {
             ADTTE_summary <- ADTTE |>
               dplyr::group_by(STUDYID, USUBJID) |>
               dplyr::summarize(PARAMCD_AVAL = paste(sprintf("%s - %.2f", PARAMCD, AVAL), collapse = "; "))
             ADSL <- dplyr::left_join(ADSL, ADTTE_summary)
           },
           nobs = input$obs
         )
       })
     })
   },
   datanames = "ADSL"
 )
)

data <- teal_data_module(
 once = FALSE,
 ui = function(id) {
   ns <- NS(id)
   tagList(
     numericInput(ns("obs"), "Number of observations to show", 1000),
     actionButton(ns("submit"), label = "Submit")
   )
 },
 server = function(id, ...) {
   moduleServer(id, function(input, output, session) {
     logger::log_trace("example_module_transform2 initializing.")
     eventReactive(input$submit, {
       data <- within(data = teal_data(), n = as.numeric(input$obs), {
         logger::log_trace("Loading data")
         z <- NULL
         ADSL <- head(teal.data::rADSL, n = n)
         ADTTE <- teal.data::rADTTE
         iris <- iris
         iris_raw <- iris
         CO2 <- CO2
         factors <- names(Filter(isTRUE, vapply(CO2, is.factor, logical(1L))))
         CO2[factors] <- lapply(CO2[factors], as.character)
       })
       join_keys(data) <- default_cdisc_join_keys[c("ADSL", "ADTTE")]
       teal.data::datanames(data) <- c("CO2", "ADTTE", "iris_raw", "iris", "ADSL")
       data
     })
   })
 }
)

mods <- modules(
 modules(
   label = "all",
   example_module("all", datanames = "all") |>
     set_datanames("ADSL"), # this will be ignored - set_datanames on the modules level overwrites all items
   example_module("null", datanames = NULL)
 ) |> set_datanames(c("ADSL", "ADTTE", "iris", "iris_raw", "CO2")),
 modules(
   label = "modules datanames",
   example_module("mod-1"),
   example_module("mod-2")
 ) |> set_datanames("ADTTE"), # should include also ADSL (parent)
 modules(
   label = "specified",
   example_module("adsl", datanames = "ADSL"),
   example_module("adsl+adtte", datanames = c("ADSL", "ADTTE")),
   example_module("adtte+parent", datanames = "ADTTE")
 ),
 modules(
   label = "transforms",
   example_module("anl - transform w/ datanames", dataname = "ANL", transformers = trans["anl_w_datanames"]),
   example_module("anl - transform w/o datanames", dataname = "ANL", transformers = trans["anl_wout_datanames"]),
   example_module("adsl - transform w/ datanames", dataname = "ADSL", transformers = trans["adsl_w_datanames"]),
   example_module("adsl - transform w/o datanames", dataname = "ADSL", transformers = trans["adsl_wout_datanames"]),
   example_module("all w/o datanames", dataname = "all", transformers = trans["anl_wout_datanames"]),
   example_module("all reset datanames", dataname = "all", transformers = trans["anl_wout_datanames"]) |> set_datanames(c("ADSL", "ADTTE", "ANL"))
 ),
 modules(
   label = "missings",
   example_module("inexisting+existing", datanames = c("ADTTE", "inexisting")),
   example_module("inexisting", datanames = "inexisting")
 ),
 reporter_previewer_module()
)

slices <- teal_slices(
 teal_slice("ADSL", "SEX"),
 teal_slice("ADSL", "AGE", selected = c(18L, 65L)),
 teal_slice("ADTTE", "PARAMCD", selected = "CRSD"),
 include_varnames = list(
   ADSL = c("SEX", "AGE")
 )
)

app <- teal::init(
 data = data,
 modules = mods,
 filter = slices
)

runApp(app)

[github-actions]

Unit Tests Summary

  1 files   25 suites   9m 8s ⏱️
254 tests 248 ✅ 6 💤 0 ❌
525 runs  519 ✅ 6 💤 0 ❌

Results for commit b8cfe4c.

♻️ This comment has been updated with latest results.

[github-actions]

Unit Test Performance Difference

Test Suite	$Status$	Time on main	$±Time$	$±Tests$	$±Skipped$	$±Failures$	$±Errors$
module_teal	💚	$40.49$	$-2.06$	$-5$	$0$	$0$	$0$
shinytest2-data_summary	💚	$36.91$	$-1.01$	$0$	$0$	$0$	$0$
shinytest2-modules	💚	$37.61$	$-1.17$	$0$	$0$	$0$	$0$
utils	💚	$68.17$	$-1.46$	$-2$	$0$	$0$	$0$

Additional test case details

Test Suite	$Status$	Time on main	$±Time$	Test Case
module_teal	👶		$+0.39$	combines_datanames_from_transform_module_datanames
module_teal	👶		$+0.39$	does_not_receive_transform_datasets_not_specified_in_transform_datanames_nor_modue_datanames
module_teal	💀	$0.41$	$-0.41$	doesn_t_receive_extra_datanames_in_a_transform_if_not_specified_in_module_datanames
module_teal	💀	$0.40$	$-0.40$	doesn_t_receive_extra_transform_datasets_not_set_in_datanames_if_module_datanames_all_
module_teal	👶		$+0.39$	receives_all_datasets_if_transform_datanames_all_
module_teal	💀	$0.62$	$-0.62$	receives_all_objects_from_env_except_DATA._raw__when_DATA_is_present_in_the_env_and_module_datanames_all_and_datanames_is_empty
module_teal	👶		$+0.60$	receives_all_objects_from_env_excluding_.dataname__raw__when_module_datanames_all
module_teal	👶		$+0.40$	receives_all_transform_datasets_if_module_datanames_all_
module_teal	💀	$0.64$	$-0.64$	receives_datanames_when_module_datanames_all_
module_teal	💀	$0.40$	$-0.40$	receives_extra_datanames_added_in_a_transform_if_specified_in_module_datanames
module_teal	💀	$0.41$	$-0.41$	receives_extra_transform_datasets_if_module_datanames_all_and_datanames_empty
module_teal	💀	$0.31$	$-0.31$	reflects_transform_adding_new_dataset
module_teal	👶		$+0.34$	reflects_transform_adding_new_dataset_if_specified_in_module
modules	👶		$+0.00$	module_datanames_is_appended_by_its_transformers_datanames
modules	👶		$+0.00$	module_datanames_is_set_to_all_if_transformer_datanames_is_all_
modules	👶		$+0.00$	module_datanames_stays_all_regardless_of_transformers
utils	💚	$68.00$	$-1.45$	create_renv_lockfile_creates_a_lock_file_during_the_execution
utils	💀	$0.00$	$-0.00$	teal_data_datanames_returns_datanames_which_are_set_by_teal.data_datanames
utils	💀	$0.00$	$-0.00$	teal_data_datanames_returns_names_of_the_env_s_objects_when_datanames_not_set

Results for commit e8a6613

♻️ This comment has been updated with latest results.

[donyunardi]

Thanks for this alternative solution.

To better illustrate the different scenarios, I've created a table based on the example app that @gogonzo provided. Just to clarify, the data passed to the module contains values in the data$datanames slot.

Scenario table

#	module$datanames	use transform?	set transform$datanames?	|> set_datanames()?	module and transform get
1	ADSL	❌	❌	❌	ADSL
2	ADSL	✅	ANL	❌	ADSL, ANL
3	ADTTE	❌	❌	❌	ADTTE, ADSL (parent)
4	ANL	✅	❌	❌	ANL
5	ANL	✅	ADSL	❌	ANL, ADSL
6	ANL	✅	ADTTE	❌	ANL, ADTTE, ADSL (parent)
7	all	❌	❌	❌	all objects in data$env
8	all	✅	ADSL	❌	all objects in data$env
9	all	✅	ANL	❌	all objects in data$env
10	NULL	❌	❌	❌	all objects in data$env
11	NULL	✅	❌	❌	all objects in data$env
12	NULL	✅	ANL	❌	ANL
13	NULL	✅	ADSL	❌	ADSL
14	NULL	✅	ADTTE	❌	ADTTE, ADSL
15	NULL	✅	❌	ANL	ANL
16	all	✅	❌	ANL	ANL
17	ADSL	✅	ANL	ADSL	ADSL
18	ADSL	✅	❌	ANL	ANL
19	ANL	✅	❌	ADSL	ADSL

We can use this as reference if needed.

[donyunardi]

What I like

single point of truth (module$datanames)
don't need to take care of data@datanames

This makes sense to me. The transform module is designed to be local to the teal module, and module$datanames is always intended to either retrieve or limit the data objects pulled from data.

module appends transform$datanames to module$datanames to always keep module$datanames as a single point of truth.

My understanding is that the purpose of datanames remains consistent for both the transform and the module, which is to either retrieve or limit the data objects pulled from data. I like this as well and it reflected nicely on the scenario table.

set_dataname()

I have some reservations about using set_datanames. While I agree it would simplify things, I wonder if it's too brute of an approach for specifying datanames. For instance, if set_datanames ultimately resets the datanames and modules only access those specified by set_datanames, why do we still need the datanames argument in modules$datanames?

If the motivation behind set_datanames is to handle cases where modules$datanames is set to 'all' or 'NULL', should we limit the use of set_datanames to just these scenarios?

data$datanames still needed?

From what I've observed, there seems to be no need to set data$datanames during the creation of teal_data(). For example, if I omit CO2 from datanames during the creation of a teal_data_module, I can still access this data by setting module$datanames = 'CO2'. Does this mean users can skip defining data$datanames altogether? I don't mind giving users one less thing to do, but I'm wondering if there's another impact we're not considering.

[gogonzo]

If the motivation behind set_datanames is to handle cases where modules$datanames is set to 'all' or 'NULL', should we limit the use of set_datanames to just these scenarios?

Good point, thanks! I think so that set_datanames() should work only if datanames are "all" or NULL.

From what I've observed, there seems to be no need to set data$datanames during the creation of teal_data(). For example, if I omit CO2 from datanames during the creation of a teal_data_module, I can still access this data by setting module$datanames = 'CO2'.

Yes, that's true. module$datanames always had and has a priority over @datanames

Does this mean users can skip defining data@datanames altogether? I don't mind giving users one less thing to do, but I'm wondering if there's another impact we're not considering.

Yes, app-developers can skip data@datanames and depend only on module$datanames. At this moment we can still support data@datanames with the intention to deprecate in the future.

[github-actions]

Code Coverage Summary

Filename                          Stmts    Miss  Cover    Missing
------------------------------  -------  ------  -------  ----------------------------------------------------------------------------------------------------------------------------------------
R/checkmate.R                        24       0  100.00%
R/dummy_functions.R                  49      11  77.55%   30, 32, 44, 55-62
R/get_rcode_utils.R                  12       0  100.00%
R/include_css_js.R                   22      17  22.73%   12-38, 76-82
R/init.R                            110      51  53.64%   107-114, 163-172, 174, 186-207, 232-235, 242-249, 252-253, 255
R/landing_popup_module.R             25      25  0.00%    61-87
R/module_bookmark_manager.R         158     127  19.62%   47-68, 88-138, 143-144, 156, 203, 238-315
R/module_data_summary.R             193      68  64.77%   24-52, 94, 192, 232-272
R/module_filter_data.R               53       2  96.23%   22-23
R/module_filter_manager.R           230      57  75.22%   56-62, 73-82, 90-95, 108-112, 117-118, 291-314, 340, 367, 379, 386-387
R/module_init_data.R                107      11  89.72%   44-53, 67
R/module_nested_tabs.R              167      70  58.08%   33-121, 150, 200, 258, 297
R/module_snapshot_manager.R         216     146  32.41%   89-95, 104-113, 121-133, 152-153, 170-180, 184-199, 201-208, 215-230, 234-238, 240-246, 249-262, 265-273, 304-318, 321-332, 335-341, 355
R/module_teal_data.R                114      21  81.58%   42-48, 87-90, 114-123
R/module_teal_with_splash.R          12      12  0.00%    22-38
R/module_teal.R                     141      92  34.75%   42-145, 182-183, 191
R/module_transform_data.R            56      32  42.86%   17-51
R/modules.R                         181      32  82.32%   166-170, 225-228, 326-327, 359-373, 411, 423-431
R/reporter_previewer_module.R        19       2  89.47%   30, 34
R/show_rcode_modal.R                 24      24  0.00%    17-42
R/tdata.R                            14      14  0.00%    19-61
R/teal_data_module-eval_code.R       24       0  100.00%
R/teal_data_module-within.R           7       0  100.00%
R/teal_data_module.R                 43       0  100.00%
R/teal_data_utils.R                  39       0  100.00%
R/teal_lockfile.R                    56      22  60.71%   34-38, 43-51, 72, 91, 94-102, 109
R/teal_reporter.R                    68       6  91.18%   69, 77, 125-126, 129, 146
R/teal_slices-store.R                29       0  100.00%
R/teal_slices.R                      63       0  100.00%
R/TealAppDriver.R                   353     353  0.00%    52-735
R/utils.R                           198       0  100.00%
R/validate_inputs.R                  32       0  100.00%
R/validations.R                      58      37  36.21%   110-377
R/zzz.R                              12       8  33.33%   3-15
TOTAL                              2909    1240  57.37%

Diff against main

Filename                  Stmts    Miss  Cover
----------------------  -------  ------  --------
R/init.R                     -2      -1  +0.06%
R/module_filter_data.R       +1       0  +0.07%
R/module_init_data.R         -1       0  -0.10%
R/modules.R                 +19     +11  -4.72%
R/teal_data_module.R         +1       0  +100.00%
R/teal_data_utils.R          -4       0  +100.00%
R/utils.R                    +5       0  +100.00%
TOTAL                       +19     +10  -0.07%

Results for commit: b8cfe4c

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

[kumamiao]

I like that we have a single point of truth module$datanames, and agree that set_datanames() should only be used when datanames are "all" or NULL.

I actually do like that app-developers can skip data@datanames, where it now seems like the functionality of setting data@datanames can be achieved by setting module$datanames.

[donyunardi]

Minor sentence update/suggestion.
Everything else looks good.

Thanks for the thinking hard on this!