introduce teal_data class by gogonzo · Pull Request #178 · insightsengineering/teal.data

gogonzo · 2023-10-18T12:47:57Z

Part of:

Use same data object for teal_data and in modules teal#937

Closes:

change new_teal_data arguments #169

Other PRs:

install

staged.dependencies::dependency_table(
  project = "insightsengineering/teal@https://github.com",
  project_type = "repo@host",
  ref = "teal_data@main",
  verbose = 1
) |> staged.dependencies::install_deps()

teal_data class

teal_data class (teal_data-class.R) is en extension of qenv. teal_data contains extra slots datanames and join_keys. Besides, all teal.code methods works with teal_data object (eval_code, print etc.)
Old TealData$server returns teal_data now - to have single object type in teal (not in teal_module)
cdisc_data() and teal_data() return TealData or teal_data

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

data <- cdisc_data(
  ADSL = ADSL,
  ADTTE = ADTTE,
  ADRS = ADRS,
  code = quote({
    ADSL <- synthetic_cdisc_data("latest")$adsl
    ADTTE <- synthetic_cdisc_data("latest")$adtte
    ADRS <- synthetic_cdisc_data("latest")$adrs
  })  
)

data |> eval_code(quote(a <- ADSL))

any data possible

adjust to teal

`ddl` implementation alternative to #161 . Complemented by [this PR](insightsengineering/teal#922). In order to simplify the user (app dev) experience, I tried to streamline the logic. In order to create a `ddl` connector module, one has to: 1. use `input_template` to create the module: enumerate input widgets 2. provide a function, `on_submit`, to be run when `"submit"` button is clicked; function takes input values wrapped in a list called `input` and body refers to input values with `input$<value>` or `input[["<value>"]]` 3. optionally provide mask for input values that will be used in code of resulting `tdata` object 4. specify names of data sets for compatibility with `teal` (I don't like it) 5. optionally specify join keys as one would previously, for compatibility with `teal`; defaults to empty `teal.data::join_keys()` When inputs are submitted, `on_submit` is passed to a function that extracts the body, substitutes `input` placeholders with input values and evaluates the code to obtain data sets in a separate environment. Then it replaces the input values in the code with ones provided in `mask` (if any) and uses the environment and the masked code to create `tdata`. Much like in the solution proposed on branch `refactor`, the user provides code to obtain data sets and replacements for input values, and data is created in separate environment, which is then used to create `tdata` with masked code. Unlike that solution, the user specifies everything in one place, rather than having to define module ui, module server that runs a post-processing function, the post-processing function itself, etc. This is easier to understand **for me**. Another difference is that the user provides code as code with `input$` references, not text with `glue` syntax (`{ value }`). This is done move focus to masking rather than have the user think about "online" and "offline" arguments. It also uses pure base R. #### MOCK DATABASE CONNECTION ``` pullme <- function(username, password) { if (username == "user" && password == "pass") { message("connection established") } else { stop("invalid credentials") } } closeme <- function() { message("connection closed") } ``` #### MODULE DEFINITION ``` library(shiny) thefun <- function(input) { on.exit(try(closeme())) pullme(username = input$user, password = input$pass) adsl <- scda::synthetic_cdisc_data('latest')$adsl adtte <- scda::synthetic_cdisc_data('latest')$adtte } themask <- list( user = quote(askpass("who are you?")), pass = quote(askpass("password please")) ) module <- input_template( on_submit = thefun, mask = themask, datanames = c("adsl", "adtte"), textInput("user", "username", value = "user", placeholder = "who goes there?"), passwordInput("pass", "password", value = "pass", placeholder = "friend or foe?"), actionButton("submit", "get it") ) ``` #### AN APP ``` devtools::load_all("../teal.slice") devtools::load_all("../teal") devtools::load_all(".") ui <- fluidPage( tagList( module$ui("id"), uiOutput("val") ) ) server <- function(input, output, session) { tdata <- module$server("id") output[["value"]] <- renderPrint({ tdata() }) output[["code"]] <- renderPrint({ cat(teal.code::get_code(tdata()), sep = "\n") }) output[["val"]] <- renderUI({ req(tdata()) tagList( verbatimTextOutput("value"), verbatimTextOutput("code") ) }) } if (interactive()) shinyApp(ui, server) ``` #### A TEAL APP ``` funny_module <- function (label = "Filter states", datanames = "all") { checkmate::assert_string(label) module( label = label, datanames = datanames, ui = function(id, ...) { ns <- NS(id) div( h2("The following filter calls are generated:"), verbatimTextOutput(ns("filter_states")), verbatimTextOutput(ns("filter_calls")), actionButton(ns("reset"), "reset_to_default") ) }, server = function(input, output, session, data, filter_panel_api) { checkmate::assert_class(data, "tdata") observeEvent(input$reset, set_filter_state(filter_panel_api, default_filters)) output$filter_states <- renderPrint({ logger::log_trace("rendering text1") filter_panel_api %>% get_filter_state() }) output$filter_calls <- renderText({ logger::log_trace("rendering text2") attr(data, "code")() }) } ) } devtools::load_all("../teal.slice") devtools::load_all("../teal") devtools::load_all(".") app <- init( data = module, modules = modules( funny_module("funny1"), funny_module("funny2", datanames = "adtte") # will limit datanames to ADTTE and ADSL (parent) ) ) shinyApp(app$ui, app$server) ```

ddl WIP

POC

tdata -> teal_data

mask_args -> input_mask

online_args -> input

ddl from teal.data to teal

teal_data class only

remove to_relational for classes which doesn't need transformation

specify datanames when more objects in environment

Fixes the broken tests due to the teal_data refactor --------- Co-authored-by: go_gonzo <dawid.kaledkowski@gmail.com>

reload NAMESPACE

tidyup docs

fix lintr

- rename keys to join_keys in new_teal_data - fill datanames by join keys datanames

lintr fix

fix tests

chlebowa · 2023-10-25T10:14:17Z

Even though this branch is green, these tests fail on my machine 🤔

Failure (test-TealDatasetConnector.R:567:3): code_dataset_connector - library calls
`data <- cdisc_data(adsl, adtte, adrs, check = TRUE)` did not produce any warnings.

Failure (test-TealDatasetConnector.R:567:3): code_dataset_connector - library calls
`data <- cdisc_data(adsl, adtte, adrs, check = TRUE)` did not produce any warnings.

move get_join_keys.default from teal

add get_join_keys for TealData

Adds function `datanames` to get or set the contents of the `@datanames` slot in `teal_data` objects. --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>

averissimo

teal_data looks very good at the current state!

Some minor comments, below.. The test/testhat/test-TealDatasetConnector.R is the only one that seems important to merge.

My only big grip is one layer down with using S4 classes, in part by having exposed slots in qenv and by consequence on teal_data class.

It seems more beneficial to keep those fields as private and using active bindings to "expose" those as read-only.

However, converting qenv to R6 is a whole new big project and may introduce caveats as well (hence the use of S4... this predates my joining of the project and I'm not familiar with all the reasons).

@averissimo

@averissimo suggestions Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com> Signed-off-by: Dawid Kałędkowski <6959016+gogonzo@users.noreply.github.com>

chlebowa · 2023-10-29T15:31:00Z

+#' The `dataname` is extrapolated from the name  (or fallback to the value itself if
+#' it's a `character(1)`).


Not entirely clear.

chlebowa · 2023-10-29T15:37:15Z

+    if (name %in% names(default_cdisc_keys)) {
+      # Set default primary keys
+      keys_list <- default_cdisc_keys[[name]]
+      join_keys[name] <- keys_list$primary
+
+      if (!is.null(keys_list$parent) && !is.null(keys_list$foreign)) {
+        join_keys[name, keys_list$parent] <- keys_list$foreign
+      }
+    }


This block should go into the block in line 424. The logic will be reflected better and the return(NULL) statements can be removed. The last case becomes redundant altogether.

Not true. If name is not null then previous if-else doesn't exit (don't return NULL)

How about this?

if (checkmate::test_class(item, "JoinKeySet")) { join_keys$set(item) } else { if ((is.null(name) || identical(trimws(name), "")) && is.character(item)) { name <- item } if (name %in% names(default_cdisc_keys)) { # Set default primary keys keys_list <- default_cdisc_keys[[name]] join_keys[name] <- keys_list$primary if (!is.null(keys_list$parent) && !is.null(keys_list$foreign)) { join_keys[name, keys_list$parent] <- keys_list$foreign } } }

On another note, I think using lapply like this is missing the point of lapply. A loop could actually be easier to read and we wouldn't have to consider return(NULL)s.

@chlebowa

@chlebowa

chlebowa · 2023-10-30T09:16:57Z

+#' @rdname teal_data-class
+#'
+#' @slot env (`environment`) environment containing data sets and possibly auxiliary variables
+#'  Access variables with [get_var()] or [`[[`].


Suggested change

#' Access variables with [get_var()] or [`[[`].

#' Access variables with [`get_var`] or [`[[`].

what is wrong with [get_var()]

It's a function call, usually docs mention function names, and in code format.

chlebowa · 2023-10-30T09:17:06Z

+#'  Access with [get_warnings()].
+#' @slot messages (`character`) messages raised when evaluating code.
+#' @slot join_keys (`JoinKeys`) object specifying joining keys for data sets in `@env`.
+#'  Access or modify with [get_join_keys()].


Suggested change

#' Access or modify with [get_join_keys()].

#' Access or modify with [`get_join_keys`].

Not according to roxygen2 docs.

See, in the rendered docs - Every [fun()] is rendered as fun() in the form of link. Topics have no () so it is better to distinguish what is a topic and what is a function.

+1 for using () in function names

The distinction between topic and function seems unclear to me. Is a function NOT a topic? What is a topic then?

Here's my view:
The docs should read "Change datanames slot with datanames function."
One changes the slot with datanames<object> <- <new_datanames> not with datanames(). If you insist on the latter, you lose consistency with [[.

Look at help pages for environment, get, assign, etc. Hardly a foo() in sight.

Plus I always thought that table is an example, not a directive.

Ok, so a topic would be a help page and one can be dedicated to a concept or a function or a group of functions or anything in between. Correct?

chlebowa · 2023-10-30T09:17:19Z

+#'  Access or modify with [get_join_keys()].
+#' @slot datanames (`character`) vector of names of data sets in `@env`.
+#'  Used internally to distinguish them from auxiliary variables.
+#'  Access or modify with [datanames()].


Suggested change

#' Access or modify with [datanames()].

#' Access or modify with [`datanames`].

I see, datanames is a topic

using [fun()] in roxygen2 docs

initialize empty teal_data

chlebowa

Let's go!

gogonzo and others added 30 commits August 14, 2023 14:11

e622149

any data possible

Merge remote-tracking branch 'origin/main' into refactor

9827de3

Merge remote-tracking branch 'origin/main' into refactor

2daf83a

ddl

bef092f

Merge bef092f into ba573f2

3f723f3

[skip actions] Restyle files

5e3c0cb

7429b55

adjust to teal

fix

e964b95

Merge e964b95 into ba573f2

1b1ebc6

[skip actions] Restyle files

3f37efe

change arg names

212806c

Merge branch 'main' into refactor

7d9e2b0

cleanup builds

7edaf2a

@anverissimo review

b30029b

Merge remote-tracking branch 'origin/main' into refactor

2248740

6504197

ddl WIP

d067b93

POC

56a7b2d

tdata -> teal_data

f3876df

mask_args -> input_mask

c407adf

online_args -> input

ddl_run - input can be of class "reactivevalues"

5443613

handling datanames information

48e0f3b

6b46e4c

ddl from teal.data to teal

2c9da20

teal_data class only

[skip actions] Roxygen Man Pages Auto Update

fc853ad

b56ee4e

remove to_relational for classes which doesn't need transformation

4ff57d0

specify datanames when more objects in environment

Fix the unit tests for the teal_data refactor (#172)

32adc7c

Fixes the broken tests due to the teal_data refactor --------- Co-authored-by: go_gonzo <dawid.kaledkowski@gmail.com>

7300049

reload NAMESPACE

gogonzo added 5 commits October 25, 2023 06:18

f3de72c

tidyup docs

996d080

fix lintr

4c03c22

- rename keys to join_keys in new_teal_data - fill datanames by join keys datanames

5936220

lintr fix

88fc42d

fix tests

gogonzo added 2 commits October 25, 2023 14:23

2355695

move get_join_keys.default from teal

bfa77c1

add get_join_keys for TealData

m7pr reviewed Oct 26, 2023

View reviewed changes

Comment thread tests/testthat/test-to_relational_data.R

chlebowa and others added 2 commits October 27, 2023 12:38

add datanames function (#181)

9c73c29

Adds function `datanames` to get or set the contents of the `@datanames` slot in `teal_data` objects. --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>

to support dynamic list

a1e092c

averissimo reviewed Oct 27, 2023

View reviewed changes

Comment thread NEWS.md Outdated

Comment thread R/get_join_keys.R

Comment thread R/get_join_keys.R

Comment thread tests/testthat/test-TealDatasetConnector.R Outdated

Apply suggestions from code review

2fdfeed

@averissimo suggestions Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com> Signed-off-by: Dawid Kałędkowski <6959016+gogonzo@users.noreply.github.com>

gogonzo enabled auto-merge (squash) October 27, 2023 14:09

chlebowa suggested changes Oct 29, 2023

View reviewed changes

@chlebowa review

4dd52bd

chlebowa reviewed Oct 30, 2023

View reviewed changes

Comment thread R/JoinKeys.R Outdated

f10d94a

@chlebowa

chlebowa reviewed Oct 30, 2023

View reviewed changes

Aleksander Chlebowski and others added 6 commits October 30, 2023 10:29

update documentation in teal_data.R

ffa1e00

[skip actions] Roxygen Man Pages Auto Update

7771928

d7b3bcd

using [fun()] in roxygen2 docs

3bc4e6b

initialize empty teal_data

[skip actions] Roxygen Man Pages Auto Update

0dc21e0

empty

5ba4f99

chlebowa approved these changes Oct 30, 2023

View reviewed changes

gogonzo merged commit e14426f into main Oct 30, 2023

gogonzo deleted the teal_data@main branch October 30, 2023 11:17

		#' The `dataname` is extrapolated from the name (or fallback to the value itself if
		#' it's a `character(1)`).

	#' Access variables with [get_var()] or [`[[`].
	#' Access variables with [`get_var`] or [`[[`].

	#' Access or modify with [get_join_keys()].
	#' Access or modify with [`get_join_keys`].

	#' Access or modify with [datanames()].
	#' Access or modify with [`datanames`].

Uh oh!

Uh oh!

Conversation

gogonzo commented Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

install

teal_data class

Uh oh!

chlebowa commented Oct 25, 2023

Uh oh!

Uh oh!

averissimo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chlebowa Oct 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chlebowa left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gogonzo commented Oct 18, 2023 •

edited

Loading

chlebowa Oct 30, 2023 •

edited

Loading