datanames slot in teal_transform_module #1327
Conversation
|
Code Coverage SummaryDiff against mainResults for commit: 3bf87ac Minimum allowed coverage is ♻️ This comment has been updated with latest results |
Unit Tests Summary 1 files 25 suites 9m 11s ⏱️ Results for commit 3bf87ac. ♻️ This comment has been updated with latest results. |
Unit Test Performance Difference
Additional test case details
Results for commit 3acef50 ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
This is a great start!
I have a few comments:
module$datanames not provided
When modules$datanames is not provided (i.e. example_module("adsl - no transformer no datanames in module"), the default value for modules$datanames is all which means the module has access to all datasets.
However, I don’t see the ANL object and only datasets in teal_data_module available to the module. Shouldn't ANL be available in the module or does the user need to specify it explicitly in module$datanames?
teal_transform_module$datanames vs module$datanames
From my understanding of the current design, when teal_transform_module$datanames is provided, it surfaces the datasets to the filter panel, allowing users to subset the datasets (and their parents if they have one), triggering the transformation to be rerun.
teal_module though will always receive only module$datanames (ADSL).
Ultimately, only the datasets specified in module$datanames are available to the module (and its parents if they have one).
Since we’re using the same term datanames, I have a slight concern that this might cause confusion for users, who might think that teal_transform_module$datanames has the same scope or purpose as module$datanames. I may be overthinking this, but I’m curious to hear what everyone else thinks.
Data Summary UI
The format of the Data Summary UI differs when the module only needs the new dataset (e.g., missing a bold header). Might need adjustment for consistency?
Only ANL
With other datasets
If
Exactly.
Exactly.
I agree, this is why I didn't just
Good point. |
I don't see Example Codeoptions(
teal.log_level = "TRACE",
teal.show_js_log = TRUE,
# teal.bs_theme = bslib::bs_theme(version = 5),
shiny.bookmarkStore = "server"
)
pkgload::load_all("../teal.data")
pkgload::load_all("../teal")
anl <- 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
)
})
})
}
)
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 <- teal_data(a = NULL) |>
within(
{
logger::log_trace("Loading data")
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)
},
n = as.numeric(input$obs)
)
join_keys(data) <- default_cdisc_join_keys[c("ADSL", "ADTTE")]
teal.data::datanames(data) <- c("CO2", "ADTTE", "iris_raw", "iris", "ADSL")
data
})
})
}
)
app <- teal::init(
data = data,
modules = list(
example_module("anl - no module$datanames provided", transformers = list(anl)),
example_module("anl - module$datanames set to all", datanames = "all", transformers = list(anl))
),
filter = teal_slices(
teal_slice("ADSL", "SEX"),
teal_slice("ADSL", "AGE", selected = c(18L, 65L)),
include_varnames = list(
ADSL = c("SEX", "AGE")
)
)
)
runApp(app)
@gogonzo Do you get a different result? SessionInfor$> sessionInfo()
R version 4.4.1 (2024-06-14)
Platform: x86_64-apple-darwin20
Running under: macOS Ventura 13.6.7
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.4-x86_64/Resources/lib/libRlapack.dylib; LAPACK version 3.12.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] teal_0.15.2.9056 teal.slice_0.5.1.9010 shiny_1.8.1.1 teal.data_0.6.0.9008 testthat_3.2.1.1 teal.code_0.5.0.9007
loaded via a namespace (and not attached):
[1] gtable_0.3.5 xfun_0.43 bslib_0.7.0 ggplot2_3.5.1 htmlwidgets_1.6.4 shinyjs_2.1.0
[7] teal.widgets_0.4.2.9017 vctrs_0.6.5 tools_4.4.1 generics_0.1.3 parallel_4.4.1 tibble_3.2.1
[13] fansi_1.0.6 pkgconfig_2.0.3 data.table_1.15.4 checkmate_2.3.1 desc_1.4.3 lifecycle_1.0.4
[19] compiler_4.4.1 brio_1.1.5 munsell_0.5.1 fontawesome_0.5.2 codetools_0.2-20 httpuv_1.6.15
[25] shinyWidgets_0.8.6 htmltools_0.5.8.1 sass_0.4.9 lazyeval_0.2.2 yaml_2.3.8 plotly_4.10.4
[31] tidyr_1.3.1 later_1.3.2 pillar_1.9.0 jquerylib_0.1.4 cachem_1.0.8 mime_0.12
[37] parallelly_1.37.1 tidyselect_1.2.1 digest_0.6.35 future_1.33.2 purrr_1.0.2 dplyr_1.1.4
[43] listenv_0.9.1 rprojroot_2.0.4 fastmap_1.1.1 grid_4.4.1 colorspace_2.1-0 cli_3.6.2
[49] logger_0.3.0 magrittr_2.0.3 pkgbuild_1.4.4 utf8_1.2.4 withr_3.0.0 teal.reporter_0.3.1.9011
[55] scales_1.3.0 promises_1.3.0 backports_1.4.1 httr_1.4.7 rmarkdown_2.26 globals_0.16.3
[61] memoise_2.0.1 evaluate_0.23 knitr_1.46 shinycssloaders_1.0.0 viridisLite_0.4.2 rlang_1.1.3
[67] Rcpp_1.0.12 xtable_1.8-4 glue_1.7.0 formatR_1.14 renv_1.0.7 pkgload_1.3.4
[73] jsonlite_1.8.8 teal.logger_0.2.0.9006 R6_2.5.1 |
Yeah, thanks for this example. I have something to fix here 👍 |
|
Thank you @gogonzo for the PoC! The logic listed makes sense.
Just wanted to echo with @donyunardi 's concern on the parameter name for
I get the same result as you do, I can only get |
|
@kumamiao @donyunardi
Same applies to the |
|
@donyunardi @kumamiao 
Note: Sidebar gets In my opinion this is getting too complicated:
|
We discussed it here.
I’m open to this approach. Syntactically, it makes sense to me. If a user specifies For now, I'm leaning toward just giving the user all objects. If we decide to move forward with this behavior, we'll need to carefully assess how it will affect all the modules that currently have IdeaHave you considered adding a new argument to the transformer to specify additional data objects that should be appended to the Code can look something like this: teal_transform_module <- function(label, ui, server, datanames, data_to_be_added_to_teal_data)User can define the value(s) of the |
I don't think anything except datanames(data) <- c(datanames(data), "added_in_transform")And I thought this makes sense back then, but after a short period of time even I forgot about this, which confirms that it datanames logic might be too complex to remember. This is why I proposed simplest solution (with all consequences) If we decide to ignore
This should be adjusted as much as possible with tidyselect. We tried to do something similar in teal.transfrorm for dynamic columns selection. We liked idea of introducing more functions, like |
|
Let me rephrase once again my thoughts. We need to know few things:
I think that above points define exactly what we need and we can discuss how provide simple logic for app-developer and not overcomplicate internal code. In this PR I've added Issue:
|
|
Hi @gogonzo , thank you so much for the quick and comprehensive summary!! Like you, I like (2) the most if we find a good way to only get needed data for I tested the current PR and things are working well, but could you please check if listed below are the expected behavior under option (2)? app exampleoptions(
teal.log_level = "TRACE",
teal.show_js_log = TRUE,
# teal.bs_theme = bslib::bs_theme(version = 5),
shiny.bookmarkStore = "server"
)
pkgload::load_all("../teal.data")
pkgload::load_all("../teal")
anl <- 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(),
{
sub_iris <- head(iris)
ANL <- dplyr::inner_join(
head(ADSL, nobs),
ADTTE[c("STUDYID", "USUBJID", setdiff(colnames(ADTTE), colnames(ADSL)))],
by = c("USUBJID", "STUDYID")
)
tmp_obj1 <- 100
tmp_obj2 <- "test"
},
nobs = input$obs
)
})
})
}
)
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 <- teal_data(a = NULL) |>
within(
{
logger::log_trace("Loading data")
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)
},
n = as.numeric(input$obs)
)
join_keys(data) <- default_cdisc_join_keys[c("ADSL", "ADTTE")]
teal.data::datanames(data) <- c("CO2", "ADTTE", "iris_raw", "iris", "ADSL")
data
})
})
}
)
app <- teal::init(
data = data,
modules = list(
example_module("NULL", datanames = NULL, transformers = list(anl)),
example_module("no call", transformers = list(anl)),
example_module("all", datanames = "all", transformers = list(anl)),
example_module("ANL", datanames = "ANL", transformers = list(anl)),
example_module("ANL+iris", datanames = c("ANL", "iris"), transformers = list(anl))
),
filter = teal_slices(
teal_slice("ADSL", "SEX"),
teal_slice("ADSL", "AGE", selected = c(18L, 65L)),
include_varnames = list(
ADSL = c("SEX", "AGE")
)
)
)
runApp(app)
|
Good point @kumamiao I need to fix it
If there is an extra conditions in (2) then it will be basically (4) At this moment, it feels like (4) is the most relevant:
|
|
Thanks for the laying down all the conditions nicely @gogonzo. Could you help me understand this statement?
When/where would user add/update So here, 
From what I observed, it doesn't look like app developer needs to add/update |
Sorry @donyunardi my bad - I didn't remove lines which were always adding everything from |
|
Closing this PR in favour of #1334 |
Closes #1298
datanamesslot to theteal_transform_moduleNow it works like this:
module$datanamesand inmodule-transformers$datanames.module$datanames,module-transformers$datanamesand everything which have been created in a transformer (for example ANL)module$datanamesWith above rules, there are following cases possible:
module$datanames = "ADSL"and adds transform which modifies ADSL (doesn't need anything else).module-transform$datanames = NULLthen onlyADSLwill be displayed in the filter panelmodule-transform$datanames = c("ADTTE", "whatever")then "ADTTE" and "whatever" will also be displayed and filtered even if not used by either transform and teal_module.teal_modulethough will always receive only module$datanames (ADSL).module$datanames = "ADSL"and adds transform which modifies ADSL based on other datasets (does need other datasets to work)module-transform$datanames = NULLthen onlyADSLwill be displayed in the filter panel and only ADSL will be filtered. Transform will still work but other datasets won't be filteredmodule-transform$datanames = c("ADTTE", "whatever")then both will be displayed, filtered and passed to transform.module$datanames = "ANL"and specifies a transform which uses "ADSL" and "ADTTE" to make ANL.module-transform$datanames = NULLthen nothing will be displayed in the filter panel as there is no information about "ADSL" and "ADTTE". Transform would still work and ANL would be created based on unfiltered ADSL and ADTTEmodule-transform$datanames = c("ADSL", "ADTTE")then everything is working as expected. ADSL and ADTTE is displayed in the filter panel and filtered are passed to transform module.app example