Reduce unwanted reactivity on filter panel changes

[vedhav]

Closes #62

Pending things before it can be undrafted:

1. Make the private$reactive_call for a given sid. Right now it will not work when sid is provided. A possible solution is to make the sid a private object and use it inside the server logic.
2. Figure out the right place to observe self$get_call() to extract the call.
3. Test for normal data.frame. CDISC data.frame and MAE object.

Example app to test:

library(shiny)
library(teal.data)
pkgload::load_all("teal.slice")

datasets <- init_filtered_data(
  list(
    iris = iris,
    ADSL = rADSL, ADAE = rADAE,
    MAE = hermes::multi_assay_experiment
  ),
  join_keys = default_cdisc_join_keys[c("ADSL", "ADAE")]
)

ui <- fluidPage(
  fluidRow(
    column(
      width = 9,
      DT::DTOutput("iris_table"),
      DT::DTOutput("adsl_table"),
      DT::DTOutput("adae_table"),
      shiny::verbatimTextOutput("mae_text")
    ),
    column(width = 3, datasets$ui_filter_panel("filter_panel"))
  )
)

server <- function(input, output, session) {
  datasets$srv_filter_panel("filter_panel")
  iris_filtered_data <- reactive(datasets$get_data(dataname = "iris", filtered = TRUE))
  adsl_filtered_data <- reactive(datasets$get_data(dataname = "ADSL", filtered = TRUE))
  adae_filtered_data <- reactive(datasets$get_data(dataname = "ADAE", filtered = TRUE))
  mae_filtered_data <- reactive(datasets$get_data(dataname = "MAE", filtered = TRUE))

  observeEvent(iris_filtered_data(), {
    print("IRIS changed!!!")
  })
  observeEvent(adsl_filtered_data(), {
    print("ADSL changed!!!")
  })
  observeEvent(adae_filtered_data(), {
    print("ADAE changed!!!")
  })
  observeEvent(mae_filtered_data(), {
    print("MAE changed!!!")
  })
  output$iris_table <- DT::renderDT(head(iris_filtered_data()))
  output$adsl_table <- DT::renderDT(head(adsl_filtered_data()))
  output$adae_table <- DT::renderDT(head(adae_filtered_data()))
  output$mae_text <- renderPrint({
    mae_filtered_data()
  })
}

shinyApp(ui, server)

[gogonzo]

Looks good already. sid problem is a minor thing - it is only for reactive-counts in FilterState

[vedhav]

@gogonzo I was trying to figure out what was broken when this sid is not passed properly and I can't seem to figure out a way to test this. The only thing I know is that it is used here

[gogonzo]

@gogonzo I was trying to figure out what was broken when this sid is not passed properly and I can't seem to figure out a way to test this. The only thing I know is that it is used here

Yu, I just noticed that when count_type = "all" it uses the same dataset for each FilterState.

What if you try to bind on a call here?
https://github.com/insightsengineering/teal/blob/5c3e3fd4380ef0fc7efc53ec1cdf4f5a7c922dd3/R/module_nested_tabs.R#L278

[github-actions]

Unit Tests Summary

  1 files   29 suites   22s ⏱️
362 tests 353 ✅ 0 💤 9 ❌
831 runs  822 ✅ 0 💤 9 ❌

For more details on these failures, see this check.

Results for commit 3ab89db.

♻️ This comment has been updated with latest results.

[gogonzo]

this is constructor - it should be outside of a reactive context. It is only because srv_teal insertUI which is wrong and will be removed sooner or later.

[vedhav]

Reverted that change 👍🏽
I was surprised that observing outside a module server worked just fine. Right now the observer is inside the srv_active which is also not correct. It should be somewhere similar to the global server init. I will try to move this to some server logic in the FilterData class.
But, this adds a new problem. The dataset is only dependant on the private$reactive_call () which needs shiny to work. So, we have some unit tests failing. As, those tests used to be reactive on the self$get_call() before.

[gogonzo]

filter-counts are broken

[gogonzo]

It won't work for filter-counts which has non-empty sid as an argument. Let me explain briefly how sid works and why you can't cache these object in FilteredDataset.

For given FilterState(s) configuration there are two kind of calls (and data) produced in the same time. When the filter is modified possibly multiple calls of data_filtered_fun are invoked. This means that this can't be cached here because this function needs to be called several times with different sid.

1. All FilterState are combined into one call and it is returned to the module. For this case your current solution works just perfect

2. reactive counts in a single (each) FilterState are calculated on data filtered by every filter but not "this" filter.

---

Why are counts so weird?
When you filter single column you'd like to still see the counts of the unfiltered ranges. If we don't show these counts user wouldn't know how many observations are outside of the filtered range.

correct	incorrect

Even more technical explaination here

example app

# options(teal.log_level = "TRACE", teal.show_js_log = TRUE)
library("teal.slice")
library(teal)
library(scda)
library(SummarizedExperiment)

data <- teal_data() |> within({
  # MAE <- hermes::multi_assay_experiment
  ADSL <- synthetic_cdisc_data("latest")$adsl
  ADSL$empty <- NA
  ADSL$logical1 <- FALSE
  ADSL$logical <- sample(c(TRUE, FALSE), size = nrow(ADSL), replace = TRUE)
  ADSL$numeric <- rnorm(nrow(ADSL))
  ADSL$categorical <- sample(letters[1:10], size = nrow(ADSL), replace = TRUE)
  ADSL$date <- Sys.Date() + seq_len(nrow(ADSL))
  ADSL$datetime <- Sys.time() + seq_len(nrow(ADSL)) * 3600 * 12

  ADSL$numeric[sample(1:nrow(ADSL), size = 10, )] <- NA
  ADSL$numeric[sample(1:nrow(ADSL), size = 10, )] <- Inf
  ADSL$logical[sample(1:nrow(ADSL), size = 10, )] <- NA
  ADSL$date[sample(1:nrow(ADSL), size = 10, )] <- NA
  ADSL$datetime[sample(1:nrow(ADSL), size = 10, )] <- NA
  ADSL$categorical[sample(1:nrow(ADSL), size = 10, )] <- NA

  ADTTE <- synthetic_cdisc_data("latest")$adtte
  ADRS <- synthetic_cdisc_data("latest")$adrs
})

teal.data::datanames(data) <- c(
  #  "MAE",
  "ADSL", "ADTTE", "ADRS"
)
teal.data::join_keys(data) <- teal.data::default_cdisc_join_keys[teal.data::datanames(data)]

app <- init(
  data = data,
  modules = list(
    example_module("mod1"),
    modules(
      example_module("mod2")
    )
  ),
  filter = teal_slices(
    teal_slice("ADSL", "numeric", id = "numeric1"),
    teal_slice("ADSL", "numeric", id = "numeric2"),
    module_specific = TRUE,
    mapping = list(
      mod1 = "numeric1",
      mod2 = "numeric2"
    ),
    count_type = "all"
  )
)

runApp(app)

[donyunardi]

@vedhav @gogonzo
Is this urgent? Otherwise, should we hold off on this movement until we finished other priorities?

[gogonzo]

@vedhav @gogonzo Is this urgent? Otherwise, should we hold off on this movement until we finished other priorities?

This is not urgent and working solution hasn't been provided. Let's deprioritize this