resolve_delayed now accepts a list #74

denisovan31415 · 2022-06-17T07:21:16Z

closes #67

resolve_delayed now accepts a list of data.frame or a single data.frame.

EDIT MAHMOUD:

Create a new module resolve which is the alternative of resolve_delayed that accepts a list of data.frame and a list of join.keys as input. The common functions have been moved into resolve.R as resolve_delayed.R will be removed in the future. resolve_delayed is updated to use resolve internally.
Update the tests to satisfy the new resolve function and add a section at the end of every test file for the tests of resolve_delayed preceded with the title # with resolve_delayed. This would not decrease the coverage throughout the coming sprint and would facilitate the removal of resolve_delayed and its tests in the future.
Clean resolve tests to not use FilteredData where not needed.
S3 methods for data_extract_srv for FilteredData and list of reactive data.frames.

STILL TO DO:

add tests and clean the code.
check validation for check_data_extract_spec_react(datasets, data_extract_spec) step that was removed.

github-actions · 2022-06-17T07:27:10Z

Unit Tests Summary

    1 files   21 suites 47s ⏱️
165 tests 164 ✔️ 1 💤 0 ❌
627 runs 626 ✔️ 1 💤 0 ❌

Results for commit fe6b481.

♻️ This comment has been updated with latest results.

R/resolve_delayed.R

mhallal1 · 2022-06-17T13:52:20Z

R/resolve_delayed.R

@@ -3,7 +3,8 @@
 #' @description `r lifecycle::badge("stable")`
 #'
 #' @param x Object of class `delayed_data` to resolve.
-#' @param datasets Object of class `FilteredData` to use for evaluation.
+#' @param datasets A named list of `data.frame` to use for evaluation.


Here (#75), I have kept datasets argument for FilteredData object and used data argument for the list of data.frames, shall we unify the name usage here also to avoid confusion for us and for the users? @gogonzo

I will will change the argument name once it has been decided

@mhallal1 has a good point

github-actions · 2022-06-17T15:58:31Z

Code Coverage Summary

Filename                          Stmts    Miss  Cover    Missing
------------------------------  -------  ------  -------  ------------------------------------------------------------------------
R/all_choices.R                       1       0  100.00%
R/call_utils.R                      156     124  20.51%   14-23, 64, 66, 68, 107-431
R/check_selector.R                   31       0  100.00%
R/choices_labeled.R                 202      61  69.80%   60, 71, 76, 87, 103, 221-225, 229-234, 264-277, 390-391, 393, 425-473
R/choices_selected.R                 81      11  86.42%   201-229, 260
R/column_functions.R                  3       3  0.00%    13-16
R/data_extract_datanames.R           32       8  75.00%   9-13, 64-66
R/data_extract_filter_module.R       92      11  88.04%   75-82, 84, 87-88, 141
R/data_extract_module.R             230      63  72.61%   3, 121, 126, 143, 146-151, 153, 172-175, 203-249, 413-415, 424, 429, 459
R/data_extract_read_module.R        122      13  89.34%   28, 32-35, 102-107, 116, 133
R/data_extract_select_module.R       32      18  43.75%   31-48
R/data_extract_single_module.R       53       2  96.23%   29, 42
R/data_extract_spec.R                32       0  100.00%
R/data_merge_module.R                44      19  56.82%   101-122
R/filter_spec.R                     186       1  99.46%   373
R/format_data_extract.R              16       1  93.75%   49
R/get_dplyr_call.R                  298       0  100.00%
R/get_merge_call.R                  283      32  88.69%   29-36, 47, 210-219, 362, 369, 385-397, 460
R/include_css_js.R                    5       0  100.00%
R/input_checks.R                     11       2  81.82%   18-19
R/merge_data_utils.R                  2       0  100.00%
R/merge_datasets.R                  154       7  95.45%   71, 123, 251-255
R/resolve_delayed.R                   8       0  100.00%
R/resolve.R                         110      44  60.00%   226-309
R/select_spec.R                      49       8  83.67%   149, 213-220
R/utils.R                            15      11  26.67%   26-39
R/zzz.R                               2       2  0.00%    2-3
TOTAL                              2250     441  80.40%

Results for commit: 164d2d3

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

gogonzo · 2022-06-20T07:14:34Z

resolve_delayed occurs in teal. Please make necessary changes. After it's done please fix data_extract for accept list of data (currently it still depends on a FilteredData)

kpagacz · 2022-06-20T07:42:04Z

The issue asked to create a new module instead of modifying the existing one. Are we settling on modifying now?

R/data_extract_filter_module.R

Polkas · 2022-06-23T10:02:53Z

R/data_extract_filter_module.R

@@ -49,7 +49,7 @@ data_extract_filter_ui <- function(filter, id = "filter") {
 #' the `vals` widget based on the input of the `col` widget.
 #'
 #' @param id (`character`) id string
-#' @param datasets (`FilteredData`) the datasets
+#' @param datasets (`list` of `reactive`) `data.frame`s
 #' @param filter (`filter_spec`) the filter generated by a call to [filter_spec()]
 #' @keywords internal
 data_extract_filter_srv <- function(id, datasets, filter) {


no validation here?

Added validation checkmate::assert_list(datasets, types = "reactive", names = "named")

Polkas · 2022-06-23T10:03:46Z

R/data_extract_filter_module.R

@@ -73,7 +73,7 @@ data_extract_filter_srv <- function(id, datasets, filter) {
            filter <<- filter
          } else if (!rlang::is_empty(input$col)) {
            choices <- value_choices(
-              datasets$get_data(filter$dataname, filtered = TRUE),
+              datasets[[filter$dataname]](),


Without a validation this could fail with an bad error message.

currently value_choices expects a data.frame not a reactive. We want to support only the reactive now or both of them.

We are passing a data.frame here to value_choices:

what if datasets[[filter$dataname]] is a data.frame not a reactive

data_extract_srv --> data_extract_single_srv --> data_extract_filter_srv is the sequence actually in function. We handle whether the content of datasets is a reactive or data.frame already in data_extract_srv so we do not have to worry downstream.

R/data_extract_filter_module.R

Polkas · 2022-06-23T10:11:45Z

R/data_extract_module.R

 #'     adsl_reactive_input <- data_extract_srv(
 #'       id = "adsl_var",
 #'       datasets = datasets,
 #'       data_extract_spec = adsl_extract
 #'     )
+#'     # using a list of reactive `data.frame` as input to `datasets`


commented code

yes, in this example i give both ways to use data_extract_srv with a FilteredData object and with a list of data.frame and a list of join_keys.

Please remove commented code or uncomment it :)

I added 1. and 2. for both options.

Polkas · 2022-06-23T10:12:48Z

R/data_extract_module.R

-data_extract_srv <- function(id, datasets, data_extract_spec) {
-  checkmate::assert_class(datasets, "FilteredData")
+data_extract_srv <- function(id, datasets, data_extract_spec, ...) {
+  checkmate::assert_multi_class(datasets, c("FilteredData", "list"))


nice to have additional validation here, if It is a list then should contains ...

you could add it in data_extract_srv.list

Check for reactive content is added in data_extract_srv.list:
checkmate::assert_list(datasets, types = "reactive", names = "named")

data_extract_srv.list <- function(id, datasets, data_extract_spec, keys, ...) { checkmate::assert_list(datasets, names = "named") checkmate::assert_list(keys, names = "named") checkmate::assert_names(names(datasets), permutation.of = names(keys)) ...

Pull the latest commit:

R/data_extract_module.R

Polkas · 2022-06-23T10:18:34Z

R/data_extract_read_module.R

          input_vals <- as.numeric(input_vals)
        }
        for (col in input_col) {
          # replace NA with NA_character_ for class consistency
          if (any(vapply(input_vals, identical, logical(1), "NA")) &&
-            anyNA(datasets$get_data(x$dataname)[col]) &&
-            !any(vapply(unique(datasets$get_data(x$dataname)[col]), identical, logical(1), "NA"))) {
+            anyNA(datasets[[x$dataname]]()[col]) &&


are we sure datasets[[x$dataname]] is a reactive?

Added this check in data_extract_read_srv:
checkmate::assert_list(datasets, types = "reactive", names = "named")

R/resolve.R

Polkas

I take only a quick look. I understand you will add some update here, then I will go second round. In this second round I will focus more on tests too.

It will be great to have a list what function expect a list of reactive data.frame or/and a list of data.frame.

Polkas · 2022-06-23T12:30:10Z

Please add testthat:: to each added test_that and expect_ calls. This is how rest of tests are mostly written.

Co-authored-by: Maciej Nasinski <[email protected]>

mhallal1 · 2022-06-24T07:24:52Z

Please add testthat:: to each added test_that and expect_ calls. This is how rest of tests are mostly written.

Done.

mhallal1 · 2022-06-24T09:23:33Z

replaced by #76 to make the branch name consistent with insightsengineering/teal#674

refactored resolve_delayed.R

b2f7897

denisovan31415 added the core label Jun 17, 2022

denisovan31415 marked this pull request as draft June 17, 2022 07:21

denisovan31415 requested a review from gogonzo June 17, 2022 07:21

denisovan31415 added 2 commits June 17, 2022 07:31

output of resolve_delayed is unchanged

a0c824d

fixed unit tests

c4b5a9e

gogonzo self-assigned this Jun 17, 2022

gogonzo reviewed Jun 17, 2022

View reviewed changes

R/resolve_delayed.R Outdated Show resolved Hide resolved

gogonzo reviewed Jun 17, 2022

View reviewed changes

R/resolve_delayed.R Outdated Show resolved Hide resolved

gogonzo reviewed Jun 17, 2022

View reviewed changes

R/resolve_delayed.R Outdated Show resolved Hide resolved

mhallal1 reviewed Jun 17, 2022

View reviewed changes

denisovan31415 added 3 commits June 17, 2022 10:09

fix tests

3a36acd

keys -> join_keys

9ea3ab7

join_keys required

e264eef

denisovan31415 marked this pull request as ready for review June 17, 2022 15:53

denisovan31415 changed the title ~~refactored resolve_delayed~~ resolve_delayed now accepts a list Jun 17, 2022

fixed sample app

a2b854d

mhallal1 added 3 commits June 20, 2022 11:09

fix example

c778580

resolve vs resolve_Delayed

b51c461

update tests

3b87a3d

mhallal1 added the blocked label Jun 21, 2022

mhallal1 and others added 5 commits June 21, 2022 10:36

styler

c721b94

update data_extract_srv

fb55f78

update tests

5b4a062

Merge branch 'main' into 67_resolve_delayed@main

d3102b6

docs

25f7b52

mhallal1 added 5 commits June 23, 2022 09:39

fix tests

32486ae

docs

801d702

styler

5959783

pkgdown

f85bd67

pkgdown

c5e124d

mhallal1 removed the blocked label Jun 23, 2022