Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce unwanted reactivity on filter panel changes #593

Closed
wants to merge 5 commits into from
Closed

Reduce unwanted reactivity on filter panel changes #593

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5b322a8
feat: react only when the filter call changes
vedhav May 23, 2024
89572a4
fix: suppress reactivity for CDISC child datasets too
vedhav May 24, 2024
4c4b081
chore: moving the observer to constructor
vedhav May 24, 2024
4c507ca
Revert "chore: moving the observer to constructor"
vedhav May 24, 2024
3ab89db
Merge branch 'main' into 62-improve-reactivity@main
donyunardi Jun 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion R/FilterStates.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -730,7 +730,7 @@ FilterStates <- R6::R6Class( # nolint
# create a new FilterState
fstate <- init_filter_state(
x = data[, slice$varname, drop = TRUE],
# data_reactive is a function which eventually calls get_call(sid).
# data_reactive is a function which eventually depends on the reactiveVal reactive_call().
# This chain of calls returns column from the data filtered by everything
# but filter identified by the sid argument. FilterState then get x_reactive
# and this no longer needs to be a function to pass sid. reactive in the FilterState
Expand Down
8 changes: 7 additions & 1 deletion R/FilteredDataset.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,11 @@ FilteredDataset <- R6::R6Class( # nolint
private$dataname <- dataname
private$keys <- keys
private$label <- if (is.null(label)) character(0) else label
private$reactive_call <- reactiveVal()

observeEvent(self$get_call(), ignoreNULL = FALSE, {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

private$reactive_call(self$get_call())
})

# function executing reactive call and returning data
private$data_filtered_fun <- function(sid = "") {
Expand All @@ -53,7 +58,7 @@ FilteredDataset <- R6::R6Class( # nolint
}
env <- new.env(parent = parent.env(globalenv()))
env[[dataname]] <- private$dataset
filter_call <- self$get_call(sid)
filter_call <- private$reactive_call()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
image
  1. reactive counts in a single (each) FilterState are calculated on data filtered by every filter but not "this" filter.
image

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)

eval_expr_with_msg(filter_call, env)
get(x = dataname, envir = env)
}
Expand Down Expand Up @@ -374,6 +379,7 @@ FilteredDataset <- R6::R6Class( # nolint
dataname = character(0),
keys = character(0),
label = character(0),
reactive_call = NULL, # reactiveVal

# Adds `FilterStates` to the `private$filter_states`.
# `FilterStates` is added once for each element of the dataset.
Expand Down
2 changes: 1 addition & 1 deletion R/FilteredDatasetDataframe.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ DataframeFilteredDataset <- R6::R6Class( # nolint
env <- new.env(parent = parent.env(globalenv()))
env[[dataname]] <- private$dataset
env[[parent_name]] <- parent()
filter_call <- self$get_call(sid)
filter_call <- private$reactive_call()
eval_expr_with_msg(filter_call, env)
get(x = dataname, envir = env)
}
Expand Down
Loading