Change dispatch method to S3 instead of custom (target: `@132_filter_panel_ui@main`) #501

averissimo · 2023-12-21T09:41:49Z

Tested with:

Overwriting one of the get_filter_call method with a blank package called {overwrite} (see below)
- The @importFrom roxygen2 tag is fundamental
Custom teal app (at the end)

note: My concern with the order of library / pkgload::load_all doesn't matter as {overwrite} imports from {teal.slice} and will re-register the method

{overwrite} package

## NAMESPACE

# Generated by roxygen2: do not edit by hand

S3method(get_filter_call,data.frame)
importFrom(teal.slice,get_filter_call)

## R/hello.R

#' A title
#' @importFrom teal.slice get_filter_call
#' @export
get_filter_call.data.frame <- function(data, states_list) {
  print("This was overwritten by {overwrite}")
}

Example of the {overwrite} in place

Note that this is non-reproducible as the {overwrite} package needs to be installed

library(teal)
#> Loading required package: shiny
#> Loading required package: teal.data
#> Loading required package: teal.code
#> Loading required package: teal.slice
#> Loading required package: teal.transform
#> Loading required package: magrittr
#> Registered S3 method overwritten by 'teal':
#>   method        from      
#>   c.teal_slices teal.slice
#> 
#> You are using teal version 0.14.0.9028
#> 
#> Attaching package: 'teal'
#> The following objects are masked from 'package:teal.slice':
#> 
#>     as.teal_slices, teal_slices

get_filter_call(data.frame(a = 1:10), "doesn't matter")
#> Error in states_list[[1L]]$get_state: $ operator is invalid for atomic vectors

library(overwrite)
#> Registered S3 method overwritten by 'overwrite':
#>   method                     from      
#>   get_filter_call.data.frame teal.slice

get_filter_call(data.frame(a = 1:10), "doesn't matter")
#> [1] "This was overwritten by {overwrite}"

Teal app

Tested with branch of {teal} 132_filter_panel_ui@main.

library(teal)

td <- teal.data::teal_data() |> within({
  new_iris = transform(iris, id = seq_len(nrow(iris)))
  new_mtcars = transform(mtcars, id = seq_len(nrow(mtcars)))
  array = array(1:3, c(2,4))
  matrix = matrix(c(1,2,3, 11,12,13, 21, 22, 23), nrow = 2, ncol = 3, byrow = TRUE,
                  dimnames = list(c("row1", "row2"),
                                  c("C.1", "C.2", "C.3")))
  matrix2 = Matrix::Matrix(1:6 + 1, nrow=3)
})
datanames(td) <- c("new_iris", "new_mtcars", "array", "matrix", "matrix2")

teal::init(
  data = td,
  modules = modules(
    example_module(label = "example teal module"),
    module(
      "Iris Sepal.Length histogram",
      server = function(input, output, session, data) {
        output$hist <- renderPlot(
          hist(data()[["new_iris"]]$Sepal.Length)
        )
      },
      ui = function(id, ...) {
        ns <- NS(id)
        plotOutput(ns("hist"))
      },
      datanames = "new_iris"
    )
  ),
  filter = teal::teal_slices(
    teal_slice(dataname = "new_iris", varname = "Species"),
    teal_slice(dataname = "new_iris", varname = "Sepal.Length"),
    teal_slice(dataname = "new_mtcars", varname = "cyl"),
    exclude_varnames = list(new_iris = c("Sepal.Width", "Petal.Width")),
    mapping = list(
      `example teal module` = "new_iris Species",
      `Iris Sepal.Length histogram` = "new_iris Species",
      global_filters = "new_mtcars cyl"
    )
  )
) |> shiny::runApp()

gogonzo · 2023-12-21T11:21:58Z

I can't override get_filter_call for data.frame. Please have a look at the example. I added a get_filter_call.data.frame and only thing I've changed comparing to method in teal.slice I've changed dplyr::filter to subset. Method I've created is never dispatched. Currently on my branch, this is working (even without registerS3method call)

options(teal.log_level = "TRACE", teal.show_js_log = TRUE, teal.bs_theme = bslib::bs_theme(version = 3))
library(shiny)
library(teal)
pkgload::load_all("teal.data")
library(teal.modules.hermes)

tdata <- teal_data() |>
  within({
    set.seed(1)
    library(scda)
    library(airway)
    library(MultiAssayExperiment)
    library(stats)
    ts_example <- ts(matrix(rnorm(300), 100, 3), start = c(1961, 1), frequency = 12)
    data(miniACC, envir = environment())
    data(airway, envir = environment())
    a_matrix <- matrix(rnorm(100), ncol = 5, dimnames = list(rownames = NULL, colnames = c("a", "b", "c", "d", "e")))
    a_vector <- "elo"
    ADSL <- synthetic_cdisc_data("latest")$adsl
    ADSL$categorical <- sample(letters[1:3], size = nrow(ADSL), replace = TRUE, prob = c(.1, .3, .6))
    ADTTE <- synthetic_cdisc_data("latest")$adtte
    ADRS <- synthetic_cdisc_data("latest")$adrs
  })

datanames(tdata) <- c("ADSL", "ADTTE", "ADRS", "miniACC", "airway", "a_vector", "a_matrix", "ts_example")
join_keys(tdata) <- default_cdisc_join_keys[c("ADSL", "ADTTE", "ADRS")]


default_filters <- teal.slice::teal_slices(
  teal_slice(dataname = "ADSL", varname = "categorical", selected = c("a"), id = "categorical", multiple = FALSE),
  count_type = "all"
)

get_filter_call.data.frame <- function(data, states_list) {
  cat("THIS IS A LOG--------------------------------------------------------------/n")
  dataname_lang <- str2lang(states_list[[1L]]$get_state()$dataname)
  states_predicate <- lapply(states_list, function(state) state$get_call())
  combined_predicate <- calls_combine_by(states_predicate, "&")
  rhs <- as.call(c(str2lang("subset"), c(list(dataname_lang), combined_predicate)))

  substitute(
    expr = dataname_lang <- rhs,
    env = list(dataname_lang = dataname_lang, rhs = rhs)
  )
}
registerS3method("get_filter_call", "data.frame", teal.slice::get_filter_call)

app <- init(
  data = tdata,
  modules = modules(
    modules(
      label = "tab1",
      example_module("funny"),
      example_module("funny", datanames = "airway"),
      example_module("funny2", datanames = "ADTTE"), # will limit datanames to ADTTE and ADSL (parent),
      teal.modules.general::tm_data_table()
    )
  ),
  filter = default_filters
)

runApp(app)

chlebowa · 2023-12-21T11:27:07Z

I can't override get_filter_call for data.frame. Please have a look at the example. I added a get_filter_call.data.frame and only thing I've changed comparing to method in teal.slice I've changed dplyr::filter to subset. Method I've created is never dispatched. Currently on my branch, this is working (even without registerS3method call)

The method is defined in the global environment, correct?

gogonzo · 2023-12-21T11:38:04Z

The method is defined in the global environment, correct?

Yes. Should it make any difference? Normally when there is some S3 generic, for example in base. You can just do following:

within.data.frame <- function(data, expr, ...) {
  stop("hihi")
}
within(iris)


obj <- structure(list(), class = "very_custom")
within.very_custom <- function(data, expr, ...) {
  stop("very hihi")
}
within.very_custom(obj)

One doesn't need to do any fancy stuff to register new S3 method or to overwrite one. @averissimo please provide reproducible example to prove that overriding existing method is possible both in the .GlobalEnv and by loading a package.

chlebowa · 2023-12-21T11:57:44Z

Just checking. In my experience this should work, in fact I have used this mechanism myself a number of times.

What happens if you add #' @export to the method and devtools::document?

gogonzo · 2023-12-21T12:43:11Z

What happens if you add #' @export to the method and devtools::document?

devtools::document("what?") - a source file with a shiny app? 😮

chlebowa · 2023-12-21T12:44:31Z

What happens if you add #' @export to the method and devtools::document?

devtools::document("what?") - a source file with a shiny app? 😮

Sorry. I mean if the definition were in the package. But that's not the point, is it?

averissimo · 2023-12-21T15:12:30Z

You're right, it seems like the dispatch method will not work as intended and overwrite the methods *within* {teal.slice} (only outside this namespace)

We would need to (forcibly) change the namespace in {teal.slice} with assignInNamespace which we shouldn't be promoting.

assignInNamespace("get_filter_call.data.frame", get_filter_call.data.frame, "teal.slice")

You're initial solution achieves this customization via a manual dispatch that can be overwritten with S3methods. I think we can close this PR

Some extra research trying to work aroud it:

Trying to find workarounds I particularly enjoyed reading the documentation ?parent.frame

Anyway, we can change the .__S3MethodsTable__. of {teal.slice} namespace with the code below, but the dispatch will still end up on the original function

library(teal.slice)
#> Loading required package: shiny

new_dispatch <- function(data, states_list) {
  cat("THIS IS A LOG --------------------------------------------------------------/n")
  # ...
}

registerS3method("get_filter_call", "data.frame", new_dispatch, envir = environment(get_filter_call))

environment(get_filter_call)[[".__S3MethodsTable__."]]$get_filter_call.data.frame
#> function(data, states_list) {
#>   cat("THIS IS A LOG --------------------------------------------------------------/n")
#>   # ...
#> }

getS3method("get_filter_call", "data.frame", envir = environment(get_filter_call))
#> function(data, states_list) {
#>   cat("Original method --------------------------------------------------------------", "\n")
#>   browser()
#>   browser()
#>   dataname_lang <- str2lang(states_list[[1L]]$get_state()$dataname)
#>   states_predicate <- lapply(states_list, function(state) state$get_call())
#>   combined_predicate <- calls_combine_by(states_predicate, "&")
#>   rhs <- as.call(c(str2lang("dplyr::filter"), c(list(dataname_lang), combined_predicate)))
#> 
#>   substitute(
#>     expr = dataname_lang <- rhs,
#>     env = list(dataname_lang = dataname_lang, rhs = rhs)
#>   )
#> }
#> <environment: namespace:teal.slice>

averissimo · 2023-12-21T15:36:21Z

Additional note: the search of S3methods have been changing since 3.4, so an overwritten method may have been possible in the past

chlebowa · 2023-12-22T17:37:53Z

Update: methods can be overwritten in S4.

I redefined get_filter_call and its methods in S4. Then I tested adding

setMethod("get_filter_call", "data.frame", function(data, states_list) {
  str2lang("foo(bar)")
})

both in the global environment and in a package that Depends on teal.slice.

In both cases I received

> shiny::reactiveConsole(enabled = TRUE)
> library(teal.slice)
Loading required package: shiny
> fd <- init_filtered_data(
+   list(iris = iris)
+ )
> fd$set_filter_state(teal_slices(teal_slice("iris", "Species", selected = "versicolor")))
> fd$get_call("iris")
[[1]]
iris <- dplyr::filter(iris, Species == "versicolor")

> # library(teal.slice.extension)     # package that contains the same definition as the one below
> setMethod("get_filter_call", "data.frame", function(data, states_list) {
+   str2lang("foo(bar)")
+ })
> fd$get_call("iris")
[[1]]
foo(bar)

So we can get rid of the non-standard dispatch on 132_filter_panel_ui@main.

What do you think @gogonzo @pawelru?

fix: change dispatch method to S3 instead of custom

a1f4533

averissimo requested a review from gogonzo December 21, 2023 09:42

averissimo changed the title ~~Change dispatch method to S3 instead of custom (@132_filter_panel_ui@main)~~ Change dispatch method to S3 instead of custom (target: @132_filter_panel_ui@main) Dec 21, 2023

gogonzo mentioned this pull request Dec 22, 2023

Simplify package #498

Closed

averissimo closed this Dec 22, 2023

chlebowa mentioned this pull request Dec 27, 2023

Use S4 dispatch for filter panel methods #505

Closed

insights-engineering-bot deleted the dispatch@132_filter_panel_ui@main branch August 11, 2024 03:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change dispatch method to S3 instead of custom (target: `@132_filter_panel_ui@main`) #501

Change dispatch method to S3 instead of custom (target: `@132_filter_panel_ui@main`) #501

averissimo commented Dec 21, 2023 •

edited

Loading

gogonzo commented Dec 21, 2023

chlebowa commented Dec 21, 2023

gogonzo commented Dec 21, 2023 •

edited

Loading

chlebowa commented Dec 21, 2023

gogonzo commented Dec 21, 2023 •

edited

Loading

chlebowa commented Dec 21, 2023

averissimo commented Dec 21, 2023 •

edited

Loading

averissimo commented Dec 21, 2023

chlebowa commented Dec 22, 2023

Change dispatch method to S3 instead of custom (target: @132_filter_panel_ui@main) #501

Change dispatch method to S3 instead of custom (target: @132_filter_panel_ui@main) #501

Conversation

averissimo commented Dec 21, 2023 • edited Loading

{overwrite} package

Example of the {overwrite} in place

Teal app

gogonzo commented Dec 21, 2023

chlebowa commented Dec 21, 2023

gogonzo commented Dec 21, 2023 • edited Loading

chlebowa commented Dec 21, 2023

gogonzo commented Dec 21, 2023 • edited Loading

chlebowa commented Dec 21, 2023

averissimo commented Dec 21, 2023 • edited Loading

Some extra research trying to work aroud it:

averissimo commented Dec 21, 2023

chlebowa commented Dec 22, 2023

Change dispatch method to S3 instead of custom (target: `@132_filter_panel_ui@main`) #501

Change dispatch method to S3 instead of custom (target: `@132_filter_panel_ui@main`) #501

averissimo commented Dec 21, 2023 •

edited

Loading

gogonzo commented Dec 21, 2023 •

edited

Loading

gogonzo commented Dec 21, 2023 •

edited

Loading

averissimo commented Dec 21, 2023 •

edited

Loading