gogonzo commented 2 months ago

Reactive `teal`

Check example with "transform modules":

1. Transform mechanism

Currently in teal reactive-data object is handed over to the teal_module's server. In the proposed solution, intermediate module is "injected" between filter-panel and teal_module to execute transform-expression on data() and returns reactive-data (data()) further to the teal_module. This keeps data-flow consistent by keeping data() as reactive-teal_data.

Technicaly, solution is quite simple. There is teal::ui_teal_transform_data and teal::srv_teal_transform_data which calls teal_transform_module (one or many) and returns reactive-data. The most important question now is where transform_module_ui and transform_module_srv should be placed.

in teal before calling a teal_module? So that teal_module receives transformed data() and teal_module's ui is not bothered by the transform ui items
in teal_module which needs xyz_ui and xyz_srv to have a new argument for teal_transfom_module

2. App developer

2.1 Writing `ui` and `srv`

App developer will be able to specify any UI-interactivity and any transform-expression. Consider following ui and server and I expect that "section 0" is clear:

example ui and server for transform module

```r transform_iris_ui <- function(id) { ns <- NS(id) bslib::card( bslib::card_body( selectInput( ns("species"), "Select iris species", choices = c("setosa", "versicolor", "virginica"), selected = c("setosa", "versicolor", "virginica"), multiple = TRUE ) ) ) } transform_iris_srv <- function(id, data) { moduleServer(id, function(input, output, session) { reactive({ data() |> within( IRIS <- dplyr::filter(IRIS, Species %in% species), species = input$species ) }) }) } ```

2.2 Applying transform_module on teal_module

teal_transform is applied on a teal_module (called further a "local scope") it means that it has to be attached to the teal_module object! Open question is how a custom transform_ui and transform_srv will be appended by the app developer:

use tm_xyz(...) |> transform_teal_data(ui = <transform ui>, server = <transform srv>),
or use tm_xyz(..., teal_transform_module(ui = <transform ui>, server = <transform srv>)

Both are in this POC and are doing the same effect - adding <transform_ui> and <transform_srv> to the teal_module object. The difference is that (1) doesn't need to add argument to tm_xyz() while (2) require tm_xyz(..., <argument for teal transfom module>).

2.3 Replacing `data_extract_spec` with `teal_transform_module`

Because data-extract has been here with us for a long time my proposition is to not touch it. I just want to demonstrate how to avoid data_extract_spec especially a filter_spec.

data extract	teal transform modul;e
filter_spec( label = "Select endpoint:", vars = "ADTTE", choices = value_choices("ADTTE", "PARAMCD", "PARAM"), selected = "OS", multiple = FALSE )	transform_adtte_ui <- function(id) { ns <- NS(id) bslib::card( bslib::card_body( selectInput( ns("paramcd"), "Select Parameter", choices = c("CRSD", "EFS", "OS", "PFS", "TNE"), selected = "OS", multiple = FALSE ) ) ) } transform_adtte_srv <- function(id, data) { moduleServer(id, function(input, output, session) { reactive({ message("Filtering adtte data") data() \|> within( ADTTE <- dplyr::filter(ADTTE, PARAMCD %in% paramcd), paramcd = input$paramcd ) }) }) }

data extract

teal transform modul;e

filter_spec(
  label = "Select endpoint:",
  vars = "ADTTE",
  choices = value_choices("ADTTE", "PARAMCD",  "PARAM"),
  selected = "OS",
  multiple = FALSE
)

transform_adtte_ui <- function(id) {
  ns <- NS(id)
  bslib::card(
    bslib::card_body(
      selectInput(
        ns("paramcd"),
        "Select Parameter",
        choices = c("CRSD", "EFS", "OS", "PFS", "TNE"),
        selected = "OS",
        multiple = FALSE
      )
    )
  )
}

transform_adtte_srv <- function(id, data) {
  moduleServer(id, function(input, output, session) {
    reactive({
      message("Filtering adtte data")
      data() |> within(
        ADTTE <- dplyr::filter(ADTTE, PARAMCD %in% paramcd),
        paramcd = input$paramcd
      )
    })
  })
}

3. Module developer

This is the hardest bit. Consider following code defining global and local filters/transforms

teal::init(
  modules = list(
    tm_module(
      transform = c(
        teal_slices(),
        teal_transform_module()
      )
    )
  ),
  transform = c(
    teal_slices(),
    teal_transform_module()
  )
)

Above is a desired API for teal applications where one would be able to specify global and local filters and transforms.

handled by teal	handled in a module
tm_xy( x = choices_selected(), y = choices_selected(), transform = ... ) xy_ui <- function(id) { } xy_srv <- function(id, data) { }	tm_xy( x = choices_selected(), y = choices_selected(), transform = ... ) xy_ui <- function(id, transform) { ui_teal_transform_data(transform) } xy_srv <- function(id, data) { new_data <- srv_teal_transfom_data(data, transform) }

handled by teal

handled in a module

tm_xy(
  x = choices_selected(),
  y = choices_selected(),
  transform = ...
)

xy_ui <- function(id) {
}

xy_srv <- function(id, data) {
}

tm_xy(
  x = choices_selected(),
  y = choices_selected(),
  transform = ...
)

xy_ui <- function(id, transform) {
    ui_teal_transform_data(transform)
}

xy_srv <- function(id, data) {
    new_data <- srv_teal_transfom_data(data, transform)
}

If you look at above table - both solutions require transforms to be included in teal_module object (technically by exposing argument in tm_xyz()). "handled by teal" is simpler for module developer as transformation will be handled in teal which will save some extra lines of code in xyz_srv and xyz_ui (and space in GUI). However it is debatable what is more consistent from the api/design perspective. Here are some statements from which we need to choose those relevant

Fundamental is: tm_xyz is a function defining a single module so everything here is in a local scope
All ui_args and srv_args should be directly delivered to ui and srv (xyz_ui and xyz_srv)
teal_module is a teal specific object and ui/srv_args is something exclusively handled by teal
xyz_ui and xyz_srv are "teal-independent" they just require data and other args when called
xyz_ui and xyz_srv responsibility is to create an output from already prepared data

gogonzo commented 2 months ago

tmg app

```r options(teal.log_level = "TRACE", teal.show_js_log = TRUE) pkgload::load_all("teal") library(teal.modules.general) library(dplyr) options(shiny.bookmarkStore = "server") nest_logo <- "https://raw.githubusercontent.com/insightsengineering/hex-stickers/main/PNG/nest.png" datasets <- c("IRIS", "MTCARS", "CO2", "AIRMILES", "TITANIC", "ADSL", "ADRS", "ADTTE", "ADLB", "ADQS") data <- teal_data_module( ui = function(id) { ns <- NS(id) shiny::div( selectInput( ns("dataset"), "Datasets", choices = datasets, selected = datasets, multiple = TRUE ), actionButton(ns("load"), "Load Data") ) }, server = function(id) { moduleServer(id, function(input, output, session) { eventReactive(input$load, { message("Loading data") data <- teal.data::teal_data() |> within({ IRIS <- iris MTCARS <- mtcars CO2 <- CO2 AIRMILES <- airmiles TITANIC <- Titanic ADSL <- teal.modules.general::rADSL %>% mutate(TRTDUR = round(as.numeric(TRTEDTM - TRTSDTM), 1)) ADRS <- teal.modules.general::rADRS ADTTE <- teal.modules.general::rADTTE ADLB <- teal.modules.general::rADLB %>% mutate(CHGC = as.factor(case_when( CHG < 1 ~ "N", CHG > 1 ~ "P", TRUE ~ "-" ))) ADQS <- teal.modules.clinical::tmc_ex_adqs %>% dplyr::filter(ABLFL != "Y" & ABLFL2 != "Y") %>% dplyr::mutate( AVISIT = as.factor(AVISIT), AVISITN = rank(AVISITN) %>% as.factor() %>% as.numeric() %>% as.factor(), AVALBIN = AVAL < 50 # Just as an example to get a binary endpoint. ) %>% droplevels() }) teal.data::datanames(data) <- datasets teal.data::join_keys(data) <- teal.data::default_cdisc_join_keys[datanames(data)] data }) }) } ) transform_iris_ui <- function(id) { ns <- NS(id) bslib::card( bslib::card_body( selectInput( ns("species"), "Select iris species", choices = c("setosa", "versicolor", "virginica"), selected = c("setosa", "versicolor", "virginica"), multiple = TRUE ) ) ) } transform_iris_srv <- function(id, data) { moduleServer(id, function(input, output, session) { reactive({ message("Filtering iris data") data() |> within( IRIS <- dplyr::filter(IRIS, Species %in% species), species = input$species ) }) }) } transform_adrs_ui <- function(id) { ns <- NS(id) bslib::card( bslib::card_body( selectInput( ns("paramcd"), "Select Parameter", choices = c("BESRPSI", "INVET", "OVRINV"), selected = "OVRINV", multiple = TRUE ), selectInput( ns("avisit"), "Select a visit", choices = c( "SCREENING", "BASELINE", "CYCLE 2 DAY 1", "CYCLE 4 DAY 1", "END OF INDUCTION", "FOLLOW UP" ), selected = "SCREENING", multiple = TRUE ) ) ) } transform_adrs_srv <- function(id, data) { moduleServer(id, function(input, output, session) { reactive({ message("Filtering adrs data") data() |> within( ADRS <- dplyr::filter(ADRS, PARAMCD %in% paramcd, AVISIT %in% avisit), paramcd = input$paramcd, avisit = input$avisit ) }) }) } transform_adtte_ui <- function(id) { ns <- NS(id) bslib::card( bslib::card_body( selectInput( ns("paramcd"), "Select Parameter", choices = c("CRSD", "EFS", "OS", "PFS", "TNE"), selected = "OS", multiple = TRUE ) ) ) } transform_adtte_srv <- function(id, data) { moduleServer(id, function(input, output, session) { reactive({ message("Filtering adtte data") data() |> within( ADTTE <- dplyr::filter(ADTTE, PARAMCD %in% paramcd), paramcd = input$paramcd ) }) }) } app <- init( data = data, filter = teal_slices( teal_slice(dataname = "IRIS", varname = "Species", multiple = FALSE) ), modules = modules( mod1 = example_module("transform by pipe") |> transform_teal_data( ui = transform_iris_ui, server = transform_iris_srv, label = "transform iris" ), mod2 = example_module( label = "transform by arg", datanames = "all", teal_transform_module( ui = transform_iris_ui, server = transform_iris_srv, label = "transform iris" ) ), tm_g_scatterplot( label = "tmg old", x = data_extract_spec( dataname = "ADRS", select = select_spec( label = "Select variable:", choices = variable_choices("ADRS"), selected = "AVAL", multiple = FALSE, fixed = FALSE ), filter = filter_spec( label = "Select endpoint:", vars = c("PARAMCD", "AVISIT"), choices = value_choices("ADRS", c("PARAMCD", "AVISIT"), c("PARAM", "AVISIT")), selected = "OVRINV - SCREENING", multiple = FALSE ) ), y = data_extract_spec( dataname = "ADTTE", select = select_spec( label = "Select variable:", choices = variable_choices("ADTTE"), selected = "AVAL", multiple = FALSE, fixed = FALSE ), filter = filter_spec( label = "Select parameters:", vars = c("PARAMCD"), choices = value_choices("ADTTE", "PARAMCD", "PARAM"), selected = "OS", multiple = TRUE ) ), color_by = data_extract_spec( dataname = "ADSL", select = select_spec( label = "Select variable:", choices = variable_choices("ADSL", c("AGE", "SEX")), selected = "AGE", multiple = FALSE, fixed = FALSE ) ) ), tm_g_scatterplot( label = "tmg new", x = data_extract_spec( dataname = "ADRS", select = select_spec( label = "Select variable:", choices = variable_choices("ADRS"), selected = "AVAL", multiple = FALSE, fixed = FALSE ) ), y = data_extract_spec( dataname = "ADTTE", select = select_spec( label = "Select variable:", choices = variable_choices("ADTTE"), selected = "AVAL", multiple = FALSE, fixed = FALSE ) ), color_by = data_extract_spec( dataname = "ADSL", select = select_spec( label = "Select variable:", choices = variable_choices("ADSL", c("AGE", "SEX")), selected = "AGE", multiple = FALSE, fixed = FALSE ) ) ) |> transform_teal_data( ui = transform_adrs_ui, server = transform_adrs_srv, label = "transform adrs" ) |> transform_teal_data( ui = transform_adtte_ui, server = transform_adtte_srv, label = "transform adtte" ) ) ) shinyApp(app$ui, app$server) ```

In the above app I'm using the same module, the only difference I made:

In the first module i specified data extract filters
in the second module I specified custom transformation

Data Extract	Data transfom module

donyunardi commented 2 months ago

I got an error message when I run the example code:

Anyone else experiencing the same thing?

SessionInfo()

```r r$> sessionInfo() R version 4.3.3 (2024-02-29) Platform: x86_64-apple-darwin20 (64-bit) Running under: macOS Ventura 13.6.3 Matrix products: default BLAS: /System/Library/Frameworks/Accelerate.framework/Versions/A/Frameworks/vecLib.framework/Versions/A/libBLAS.dylib LAPACK: /Library/Frameworks/R.framework/Versions/4.3-x86_64/Resources/lib/libRlapack.dylib; LAPACK version 3.11.0 locale: [1] en_US.UTF-8/en_US.UTF-8/en_US.UTF-8/C/en_US.UTF-8/en_US.UTF-8 time zone: America/Los_Angeles tzcode source: internal attached base packages: [1] stats graphics grDevices utils datasets methods base other attached packages: [1] dplyr_1.1.4 teal.modules.general_0.3.0 teal.transform_0.5.0 ggmosaic_0.3.3 ggplot2_3.5.0 bslib_0.7.0 [7] rvest_1.0.4 shinytest2_0.3.1 teal_0.15.2.9031 testthat_3.2.1 teal.slice_0.5.0 teal.data_0.5.0 [13] teal.code_0.5.0 shiny_1.8.1.1 loaded via a namespace (and not attached): [1] rstudioapi_0.16.0 jsonlite_1.8.8 magrittr_2.0.3 TH.data_1.1-2 [5] estimability_1.5 rmarkdown_2.26 geepack_1.3.10 fs_1.6.3 [9] vctrs_0.6.5 memoise_2.0.1 htmltools_0.5.8.1 usethis_2.2.3 [13] polynom_1.4-1 broom_1.0.5 sass_0.4.9 htmlwidgets_1.6.4 [17] desc_1.4.3 fontawesome_0.5.2 sandwich_3.1-0 zoo_1.8-12 [21] emmeans_1.10.1 plotly_4.10.4 cachem_1.0.8 tern_0.9.3.9031 [25] mime_0.12 lifecycle_1.0.4 teal.widgets_0.4.2 pkgconfig_2.0.3 [29] colourpicker_1.3.0 Matrix_1.6-5 R6_2.5.1 fastmap_1.1.1 [33] rbibutils_2.2.16 digest_0.6.35 colorspace_2.1-0 ps_1.7.6 [37] rprojroot_2.0.4 pkgload_1.3.4 crosstalk_1.2.1 fansi_1.0.6 [41] httr_1.4.7 compiler_4.3.3 remotes_2.5.0 withr_3.0.0 [45] backports_1.4.1 bsicons_0.1.2 logger_0.3.0 pkgbuild_1.4.4 [49] MASS_7.3-60.0.1 quantreg_5.97 sessioninfo_1.2.2 ggpp_0.5.6 [53] tools_4.3.3 chromote_0.2.0 httpuv_1.6.15 glue_1.7.0 [57] callr_3.7.6 nlme_3.1-164 promises_1.3.0 grid_4.3.3 [61] checkmate_2.3.1 generics_0.1.3 teal.modules.clinical_0.9.0.9016 gtable_0.3.4 [65] tidyr_1.3.1 websocket_1.4.1 data.table_1.15.4 xml2_1.3.6 [69] utf8_1.2.4 ggrepel_0.9.5 pillar_1.9.0 stringr_1.5.1 [73] ggExtra_0.10.1 later_1.3.2 splines_4.3.3 tern.gee_0.1.3.9004 [77] lattice_0.22-6 survival_3.5-8 SparseM_1.81 tidyselect_1.2.1 [81] miniUI_0.1.1.1 rtables_0.6.6.9011 knitr_1.45 ggpmisc_0.5.5 [85] teal.logger_0.2.0 xfun_0.43 devtools_2.4.5 brio_1.1.4 [89] DT_0.33 stringi_1.8.3 lazyeval_0.2.2 yaml_2.3.8 [93] codetools_0.2-20 shinyWidgets_0.8.4 evaluate_0.23 tibble_3.2.1 [97] cli_3.6.2 xtable_1.8-4 Rdpack_2.6 munsell_0.5.1 [101] processx_3.8.4 jquerylib_0.1.4 Rcpp_1.0.12 teal.reporter_0.3.1 [105] MatrixModels_0.5-3 ellipsis_0.3.2 profvis_0.3.8 urlchecker_1.0.1 [109] formatters_0.5.5.9020 mvtnorm_1.2-4 viridisLite_0.4.2 scales_1.3.0 [113] purrr_1.0.2 crayon_1.5.2 rlang_1.1.3 multcomp_1.4-25 [117] formatR_1.14 shinyjs_2.1.0 ```

vedhav commented 2 months ago

@donyunardi could be because some of the packages are release versions. I used the dev version of all packages and did not get this error.

donyunardi commented 2 months ago

Thank you @gogonzo for preparing the PoC.

Here's my feedback:

1. Transform mechanism

The mechanism currently being proposed implies that the data will be transformed globally throughout the teal session because the data transformation is occurring between the filtered data and teal_module. If this is true, then I don't understand the purpose of making the data transformation with global scope.

To my understanding, the post-processing data transformation is only applicable in the related teal_module. If app developers would like to apply the data transformation globally, wouldn't it make more sense to use teal_data_module() and have the data be modified before passing it to the filter panel?

I understand that using teal_data_module may cause the user to be unable to interact with the transformation during exploration, which I think is the reason why the solution wants to place the transformation between the filter panel and teal_module. However, I don't see the benefit for users to be able to edit the transformation if it's being applied globally. If an app developer only wants the data transformation to apply in a specific teal_module, it's actually going to be hard to do this.

In my opinion, the scope of the post-processing data transformation should only occur locally in the teal_module. I guess this means I'm leaning toward this statement:

in teal_module which needs xyz_ui and xyz_srv to have a new argument for teal_transfom_module

Here's a diagram to illustrate my thoughts:

graph LR;
    A[Filter Panel] --data()--> B[teal_module];
    B --data()--> C[transformation module];
    C --data()--> B;
    B --generate--> D[output];

2. App developer

2.2 Applying `transform_module` on `teal_module`

To further my point earlier, where I think the data transformation should occur only locally in the teal module, I am leaning toward adding a new argument to the module.

or use tm_xyz(..., teal_transform_module(ui = , server = )

The data transformation module should be optional and when provided, would be the executed before/after the built-in (or default) transformation in the teal_module

2.3 Replacing `data_extract_spec` with `teal_transform_module`

We're diving into the better way to define select/filter spec which we will use to merge multiple datasets into one (ANL). This was categorized under App Developer but I think this could also affect Modules Developer since data_extract_spec is generally occur inside teal_module.

I like the proposed here where we're going back to the basic of shiny modules programming by defining ui and server for elements that teal module developers want to select or filter. Developers do have to write more code compared to the data_extract_spec solution. I think this is okay because most of teal module developers should already familiar with the shiny modules programming.

2.1 Writing ui and srv

Similar to point 2.3, I support the idea of app developers writing their own ui and server for UI interactivity. However, I was thinking that the object that's being passed in the tm_xyz(..., teal_transform_module=<object>) is additional transformation (data transformation or UI interactivity) that user wants to add to the related teal_module.

tm_xyz() should already have a default transformation by default, and we're extending the module by allowing additional transformations to be included in it.

3. Module developer

I would prefer the solution in which the transform is handled within the module. I would also think that teal_slices should be dedicated solely to the filter panel and not mixed with the transformation.

In my head, the ideal code would look like this:

additional_transform <- teal_transform(ui, server) # returning teal_transform object

teal::init(
  modules = modules(
    tm_module(
      arm_var = ,
      additional_transform = additional_transform
    )
  ),
  filter = teal_slices(
    teal_slice()
  )
)

vedhav commented 2 months ago

Here are some of my thoughts.

1. Transform mechanism

I think right now the complete teal_data is being passed to the transform module, so even the datanames not in used are being passed.

Having no splash and direct init is a big change, Nice feature. Have to check bookmarking.

Now that we have this module-lock feature when teal_data_module is provided, Is is possible to manage which modules can be activated based on teal_data_module? It would be a nice feature to allow an app developer to activated and display with modules the user wants.

We have abstract the transform UI card creation inside teal so the user just have to create div with the selection UI.

With all these sidebar items I wonder if it would make sense to give the app developer flexibility to initiate the sidebar with expanded/collapsed sections of the filter/transform ui. It will also be good to remember the toggle state of the UI using cookies.

I think it will be nice if we can come up with a better seperation of global and local filter/transform UI. Perhaps a separate teal tab where the global filters can live.

2. App developer

Huge win for app developers. I can finally create transform module without getting confused.

2.2 Applying `transform_module` on `teal_module`

I prefer the option 2 because it can help in providing better docs about the module. The order of data is also weird in the option 1. The teal module is called first and then transformation module is called. But, the data flows the other way.

2.3 Replacing `data_extract_spec` with `teal_transform_module`

The shiny module way of specifying the transformation is awesome. Now it adds an additional thing for app developers to learn. And shiny modules is an advanced topic for many shiny developers too, so we need to create better docs and vignettes for creating transformation modules. I think we also need some kind of a helper to just run the transform module and have a look at the UI and the data output after the transform.

3. Module developer

This API seems like a perfect way to manage local and global filters + transforms. And, placing teal_slices and teal_transform_module makes sense. But, I would prefer to separate them because teal builds the filter panel with just filters being specified by the user while the transform panel completely built by the app user. So, having filters parameter to init makes more sense to me. Additionally, a global transform module could be easily achieved by just modifying the teal_data itself, so I don't see the real utility to it. If other modules use the same transformation, it will be nice to pass the transformation to the required modules explicitly.

kumamiao commented 2 months ago

Thank you @gogonzo for the great work, and the very detailed examples and list of discussion points.

I like that we are finally touching data transformation for a better solution. Just wanted to get a very general clarification on the scope of this PR. My understanding is that this improves the functionalities of the data transformation, but my understanding is that this does not address the issue here on enabling the app user to perform any "unplanned" post-processing. For example, if the app developer did not know the app user wants to combine drug A + Drug B together in advance (therefore did not specify this transformation in the app), and then the user still does not have the capability of combining them together when exploring. Is that true or am I missing anything here?

OK now here are some of my naive thoughts on this PoC:

1. Transform mechanism

I agree with @donyunardi that global data transformation can be achieved by pre-processing the data before feeding into teal, and can be handled by the app developer. To me, it's more like global filtering is handled by teal.slice, and if any additional data manipulation (filtering, transposing, merging, etc.) needed during the analysis, then teal.transform takes care of it.

As an app developer, if I need to specify the ways to transform data in my app, pre-processing the data first does not make too much difference (or even easier)?

For an app user, other than the above clarification on "unplanned" post-processing, I do not see a strong need for a teal app to perform global data transformation beyond data filtering, which is already handled by teal_slices.

With that, I agree with @donyunardi that the transform step should go on the module level instead of globally. Similar transformation can be applied to a selection of modules if needed (i.e.,, PARAM selection can be applied to tm_g_km and tm_t_tte).

Overall I feel like the transform part will replace some contents within modules such as some TMC modules' encoding, which means updating corresponding module packages (good on the engineering perspective though).

2. App developer

2.2 Applying `transform_module` on `teal_module`

Prefers option 2. Since I think the transformation should be on module level, agree with @donyunardi that modules should come with defaults, and transformation module argument should be optional.

2.3 Replacing `data_extract_spec` with `teal_transform_module`

No strong preference, will leave it to the Coredev team.

3. Module developer

I feel like in the proposed API, app developers like me may also have confusion in terms of the overlapping functionalities and usages for using teal_slices() and teal_transform_module(). I would prefer leaving teal_slices to handle all filtering like mentioned in #1.

m7pr commented 2 months ago

I can understand the need for global transformations to be allowed, so that you

don't need to apply the same transformation to all modules if you want it to be applied to all of them
situation when data is not available during app preparation process, but only after the data is launched (e.g. data pulled from a database after credentials are provided interactively through the UI)

However I think local transformations will be the most common use case. I like the idea introduced in here https://github.com/insightsengineering/teal/pull/1212 where we allow to put transformers as separate UI and SERVER to a module through a new parameter, especially because we allow not only for data manipulation but also we open up possibilities to make changes to any part of the module (changing the form of the output, putting new inputs). This way we will be able to build up new enhanced modules, based on already existing modules, without the need to actually create a new module.

If we only go with the approach introduced here, where we allow a module (used within a module or after a module) to only transform the data, soon we will get requests to introduce modules transforming outputs (plots/tables) or overall behavior of the module. With https://github.com/insightsengineering/teal/pull/1212 we are very flexible and can allow the whole module to be changed/transformed - not only the data

gogonzo commented 2 months ago

data will be transformed globally throughout the teal session because the data transformation is occurring between the filtered data and teal_module. If this is true, then I don't understand the purpose of making the data transformation with global scope.

@donyunardi proposed mechanism in this PR transforms the data in the module scope. If we assume two scopes global and local (teal_module's scope) then API should look like this:

init(
  example_teal_module(
    transform = list() # local scope transforms
  ),
  transform = list() # global scope transforms
)

I think everyone agrees that global transforms should be applied globally for all modules and local scope should be applied locally. In this PR I exposed transform in the example_module and it is applied in the teal_module's scope.

To my understanding, the post-processing data transformation is only applicable in the related teal_module. If app developers would like to apply the data transformation globally, wouldn't it make more sense to use teal_data_module() and have the data be modified before passing it to the filter panel?

If we consider filter as a transform then restricting order of the is a mistake. In theory if global transform and local transform is a list then the order of the items in the list should be free for the user. I consider something like this:

init(
  transforms = list(
    <1. some mutation>
    <2. some filtering throught teal_slice>
  ),
  modules = example_module(
    transforms = list(
      <3. some filtering>
      <4. some mutation>
      <5. some merging>
    )
  )

)

In my opinion, the scope of the post-processing data transformation should only occur locally in the teal_module.

Yes, it means we don't implement transform in the teal::init and keep transform only in teal_module.

I would prefer the solution in which the transform is handled within the module. I would also think that teal_slices should be dedicated solely to the filter panel and not mixed with the transformation.

What about local filters?

gogonzo commented 2 months ago

@vedhav you got the point.

Having no splash and direct init is a big change, Nice feature. Have to check bookmarking.

I'm glad you've noticed this. Don't you think it is strange that splash module disappears and is not accessible anymore in the teal@main. Imagine "data" module could be a modal shown by clicking icon somewhere instead of a tab. Everything here is possible. I just wanted to expose that teal could be interactive from data->output.

Is is possible to manage which modules can be activated based on teal_data_module?

Why not - everything can be controlled and asserted. It is a matter of relevant reactive steps

I think it will be nice if we can come up with a better seperation of global and local filter/transform UI.

This is the whole idea, I'm glad you pointed it out. This can be solved either by differentiating global-local scope (for example with icon or color). Alternatively (Paweł's solution) we just put global in one panel and "local" in the modules' encoding.

gogonzo commented 2 months ago

My understanding is that this improves the functionalities of the data transformation, but my understanding is that this does not address the https://github.com/insightsengineering/coredev-tasks/discussions/533 on enabling the app user to perform any "unplanned" post-processing

@kumamiao IMO it addresses the issue. App developer can create extra teal_transform_module which handles ARM level combination or something else.

To me, it's more like global filtering is handled by teal.slice, and if any additional data manipulation (filtering, transposing, merging, etc.)

Why not teal_slice to be a wrapper around "transform module" theoretically, filtering is a data transformation which could be used alternately with teal_transform. If we have a global and local filters, then the public API for the module looks kind of messy:

init(
  modules = list(
    example_module(
      ..., 
      transform = ..., 
    )
  ),
  filter = teal_slices(
    ...
    module_specific = TRUE,
    mapping = list(
      <module1 label> = <ids of the filters>,
      ...
    )
  )

)

I would prefer leaving teal_slices to handle all filtering like mentioned in https://github.com/insightsengineering/teal/pull/1.

What about module specific filters?

gogonzo commented 2 months ago

@donyunardi @kumamiao @vedhav @m7pr

What are you thinking about such API

init(

  # global scope
  transform = list(
    # any order allowed
    teal_transform_module(),
    teal_slices(...),
    teal_transfrom_module(...)
  ),

  modules = modules(
    tm_xyz(
      # local scope
      transform = list(
        teal_slices(...),
        teal_transform_module(...)
      )
    )
  )
)

Imagine that above transformers will be executed in global/local scope in a respective order provided in a list.

vedhav commented 2 months ago

What are you thinking about such API

I think this is feature-rich, and provides more customizability. The thing that is not clear to me is that teal_slices is something that teal user can actively modify (add/remove the filters to change the widgets) while transform is not like that, the transform UI is fixed and cannot be completely removed. This provides a challenge to add them together. Perhaps we need to break down the filter-panel UI completely. The active filter panel can live with the teal transform UI. And, adding new filters UI can be somewhere else (like a popup) to make it easy to manipulate the filter/transform widgets. Additionally, do you think it is worth allowing the user to reorder the order of filter/transform? There are a few drag-and-drop UI packages with sortable being one of my favorites.

kartikeyakirar commented 2 months ago

I think there could be utility in having global transformations . Similar to how global filtering is managed with teal_slices, there's potential value in applying consistent global transformations across multiple modules.

Potential Approach The approach introduced in #1212 is straightforward and has less learning curve.

Regarding API design concerns raised, I was thinking to structure it similar to how we define teal_slices:

transform = list(
  teal_transform_module(id = <x1>, ...),
  teal_transform_module(id = <x2>, ...),
  teal_transform_module(id = <x3>, ...),
  mapping = list(
    module1 = c(<x1>, <x2>),
    module2 = c(<x2>),
    global_filters = <x3>
  )
)

there is still be issue of duplicate label name (which is may be another discussion) but we have mapping in teal_slice. this could be easily adoptable as developer already familiar.

gogonzo commented 2 months ago

The thing that is not clear to me is that teal_slices is something that teal user can actively modify (add/remove the filters to change the widgets) while transform is not like that

@vedhav teal_slices() above is a wrapper which generates a shiny module - similarily to teal_transform. This would require refactor of teal.slice for sure.

Additionally, do you think it is worth allowing the user to reorder the order of filter/transform? There are a few drag-and-drop UI packages with sortable being one of my favorites.

I'm not sure if this should be allowed. Consider scenario:

spread(data, key = "factor", value = "value")
select(reshaped_data, level1, level2)

Changing above order would cause the error

gogonzo commented 2 months ago

transform = list(
  teal_transform_module(id = <x1>, ...),
  teal_transform_module(id = <x2>, ...),
  teal_transform_module(id = <x3>, ...),
  mapping = list(
    module1 = c(<x1>, <x2>),
    module2 = c(<x2>),
    global_filters = <x3>
  )
)

@kartikeyakirar this is a interesting and constistent API to only have a transform in teal::init and keep teal_modules unchanged.

gogonzo commented 1 month ago

Separate discussion about desired API - https://github.com/insightsengineering/teal/discussions/1216

m7pr commented 1 month ago

I like the idea @kartikeyakirar proposed with mapping and reusing the mechanism for filtering. The biggest pro is that it is known and we have solid foundations to be reused. It unifies 2 approaches; for filtering and for mutating/selecting/grouping. I think we can live with inability to track separately modules if they have different labels. It also does not require users to edit teal_modules. The possibility to pass transformations into modules IMHO makes the code a lot more clutter and complicated. With propsed approach we ca still distinguish between local and global transformations

insightsengineering / teal

WIP reactive teal transform #1209

Reactive `teal`

1. Transform mechanism

2. App developer

2.1 Writing `ui` and `srv`

2.2 Applying transform_module on teal_module

2.3 Replacing `data_extract_spec` with `teal_transform_module`

3. Module developer

1. Transform mechanism

2. App developer

2.2 Applying `transform_module` on `teal_module`

2.3 Replacing `data_extract_spec` with `teal_transform_module`

2.1 Writing ui and srv

3. Module developer

1. Transform mechanism

2. App developer

2.2 Applying `transform_module` on `teal_module`

2.3 Replacing `data_extract_spec` with `teal_transform_module`

3. Module developer

1. Transform mechanism

2. App developer

2.2 Applying `transform_module` on `teal_module`

2.3 Replacing `data_extract_spec` with `teal_transform_module`

3. Module developer

insightsengineering / teal

WIP reactive teal transform #1209

Reactive teal

1. Transform mechanism

2. App developer

2.1 Writing ui and srv

2.2 Applying transform_module on teal_module

2.3 Replacing data_extract_spec with teal_transform_module

3. Module developer

1. Transform mechanism

2. App developer

2.2 Applying transform_module on teal_module

2.3 Replacing data_extract_spec with teal_transform_module

2.1 Writing ui and srv

3. Module developer

1. Transform mechanism

2. App developer

2.2 Applying transform_module on teal_module

2.3 Replacing data_extract_spec with teal_transform_module

3. Module developer

1. Transform mechanism

2. App developer

2.2 Applying transform_module on teal_module

2.3 Replacing data_extract_spec with teal_transform_module

3. Module developer

Reactive `teal`

2.1 Writing `ui` and `srv`

2.3 Replacing `data_extract_spec` with `teal_transform_module`

2.2 Applying `transform_module` on `teal_module`

2.3 Replacing `data_extract_spec` with `teal_transform_module`

2.2 Applying `transform_module` on `teal_module`

2.3 Replacing `data_extract_spec` with `teal_transform_module`

2.2 Applying `transform_module` on `teal_module`

2.3 Replacing `data_extract_spec` with `teal_transform_module`