Teal refactor #598

pawelru · 2022-09-15T16:39:08Z

FYI @shajoezhu once this goes in SME will be able to test the refactor hasn't broken anything

* Implement quosure and data to the module Co-authored-by: Mahmoud Hallal <mahmoud.hallal@roche.com> Co-authored-by: Maciej Nasinski <nasinski.maciej@gmail.com> Co-authored-by: Nikolas Burkoff <nikolas.burkoff@roche.com>

pawelru · 2022-09-15T16:49:11Z

R/tm_a_mmrm.R

-        mapply(expression = res, id = paste(names(res), "call", sep = "_"), plot_stack_push)
-        teal.code::chunks_push_chunks(plot_stack)
-        teal.code::chunks_safe_eval()
+    all_code <- shiny::reactive({


rename to all_q for consistency

I will rename all reactives returning qenv to have _q suffix.

pawelru · 2022-09-15T16:51:25Z

R/tm_a_mmrm.R

-      table_stack_push <- function(...) {
-        teal.code::chunks_push(..., chunks = table_stack)
-      }
+      q1 <- mmrm_fit()


why "1"? do we have q2 there?

This is repeated multiple times for other modules as well.

I will fix all the qenv objects to qenv, qenv1, ... etc.

R/tm_g_barchart_simple.R

pawelru · 2022-09-15T17:07:52Z

R/tm_g_barchart_simple.R

-      chunk$push(
-        bquote({
-          attr(counts[[.(get_n_name(groupby_vars))]], "label") <- "Count" # for plot
-          counts <- counts %>%
-            dplyr::group_by_at(.(as.vector(groupby_vars))) %>%
-            dplyr::slice(1) %>%
-            dplyr::ungroup() %>%
-            dplyr::select(.(as.vector(groupby_vars)), dplyr::starts_with("n_"))
-        }),
-        id = "add_label_n_group_by_call"
+      quo3 <- teal.code::eval_code(
+        quo2,
+        as.expression(
+          c(
+            bquote(attr(counts[[.(get_n_name(groupby_vars))]], "label") <- "Count"),
+            bquote(
+              counts <- counts %>%


previously it was bquote({... <\n> ...}) and you converted this into as.expression(c(buote(...), bquote(...))) and yours is more complex. May I ask what was the motivation?

I'm trying to avoid the { in the code. I'll change it to the

qenv1 <- eval_code() qenv2 <- eval_code(qenv1)

R/tm_g_barchart_simple.R

pawelru · 2022-09-15T17:22:40Z

R/tm_g_forest_rsp.R

@@ -583,7 +594,7 @@ srv_g_forest_rsp <- function(id,

      do.call(what = "validate_standard_inputs", validate_args)

-      teal::validate_one_row_per_id(anl_m$data(), key = c("USUBJID", "STUDYID", input_paramcd))
+      teal::validate_one_row_per_id(q1[["ANL"]], key = c("USUBJID", "STUDYID", input_paramcd))


few lines above you did: anl <- q1[["ANL"]] so feel free to use it

WDYT? would it be more convenient to do anl_merged_df <- reactive(anl_merged_q()[["ANL"]]) and then use that reactive clause throughout the server?

pawelru · 2022-09-15T17:32:28Z

R/tm_g_forest_rsp.R

-        list(arm_var, paramcd, subgroup_var, strata_var)
-      ),
-      modal_title = label
+      verbatim_content = reactive(teal.code::get_code(output_q())),


WDYT? would it be more convenient to do q_code <- reactive(teal.code::get_code(output_q())) and use it throughout the server?
I am looking for creating some kind of template for module that is easy to follow by module developers

pawelru · 2022-09-16T08:47:24Z

R/tm_t_summary.R

+    merged <- list(
+      anl_input_r = anl_merged_input,
+      adsl_input_r = adsl_merged_input,
+      anl_q_r = anl_merged_q
+    )


so this is yet another way of managing objects
please make it more consistent across modules

pawelru · 2022-09-16T08:50:41Z

R/tm_t_tte.R

-      adsl_filtered <- datasets$get_data(parentname, filtered = TRUE)
-      anl_filtered <- datasets$get_data(dataname, filtered = TRUE)
+      adsl_filtered <- data[[parentname]]()
+      anl_filtered <- data[[dataname]]()


is it correct to use data variable here? In other modules you get filtered datasets from merged_q variable

* fixes after changes in teal.code * Update R/tm_t_ancova.R Co-authored-by: Maciej Nasinski <nasinski.maciej@gmail.com>

Signed-off-by: Nikolas Burkoff <nikolas.burkoff@roche.com>

github-actions · 2022-10-27T13:04:02Z

Unit Tests Summary

    1 files   32 suites 4s ⏱️
140 tests 140 ✔️ 0 💤 0 ❌
155 runs 155 ✔️ 0 💤 0 ❌

Results for commit 4c9531f.

♻️ This comment has been updated with latest results.

gogonzo · 2022-11-04T12:02:22Z

I'm taking over the issue to address all the comments - please contact me before if you are planning to commit.

# Pull Request This one line ``` name = "add_label_n_group_by_call" ``` cause the module to crash: ``` example(tm_g_barchart_simple, package = 'teal.modules.clinical') shinyApp(app$ui, app$server) Error in teal.code::eval_code: unused argument (name = "add_label_n_group_by_call") ``` Removing it helps, but have a look to be sure. P.S. it is PR number 666 \m/ Signed-off-by: Marek Blazewicz <110387997+BLAZEWIM@users.noreply.github.com>

…eal.modules.clinical into teal_refactor@main

gogonzo · 2022-11-04T15:10:22Z

@pawelru your comments will be addressed here after we merge to main early next week

gogonzo

UAT passed - we addressed all functional issues.
@ruckip comments to be addressed here #667

gogonzo and others added 2 commits September 7, 2022 10:35

26 new chunks@main (#540)

2b616fa

* Implement quosure and data to the module Co-authored-by: Mahmoud Hallal <mahmoud.hallal@roche.com> Co-authored-by: Maciej Nasinski <nasinski.maciej@gmail.com> Co-authored-by: Nikolas Burkoff <nikolas.burkoff@roche.com>

Merge branch 'main' into teal_refactor@main

055d4b6

pawelru commented Sep 15, 2022

View reviewed changes

R/tm_g_barchart_simple.R Outdated Show resolved Hide resolved

pawelru commented Sep 15, 2022

View reviewed changes

R/tm_g_barchart_simple.R Outdated Show resolved Hide resolved

pawelru commented Sep 15, 2022

View reviewed changes

pawelru commented Sep 16, 2022

View reviewed changes

Nikolas Burkoff and others added 10 commits September 20, 2022 09:07

fixes after changes in teal.code (#600)

7551c54

* fixes after changes in teal.code * Update R/tm_t_ancova.R Co-authored-by: Maciej Nasinski <nasinski.maciej@gmail.com>

rename to qenv (#609)

5e189b1

Merge branch 'main' into teal_refactor@main

dbde42b

fix after merge

8c07624

Merge remote-tracking branch 'origin/main' into teal_refactor@main

7a0f1fb

Merge branch 'main' into teal_refactor@main

50e2a7f

fix conflict

a07fd64

Merge branch 'main' into teal_refactor@main

3e76a27

qenv warnings (#621)

3834ac2

Update NEWS.md

d742779

Signed-off-by: Nikolas Burkoff <nikolas.burkoff@roche.com>

nikolas-burkoff changed the title ~~Teal refactor@main~~ Teal refactor Oct 27, 2022

nikolas-burkoff marked this pull request as ready for review October 27, 2022 12:47

nikolas-burkoff mentioned this pull request Oct 27, 2022

Merge the refactor branch insightsengineering/teal#731

Closed

8 tasks

Nikolas Burkoff added 4 commits October 27, 2022 14:21

trigger

7361126

lintr

2d497a5

lintr and styler

a0776f6

styler

2484175

nikolas-burkoff added the core label Nov 3, 2022

BLAZEWIM and others added 6 commits November 4, 2022 14:01

fix bug

7ba3e22

improve mmrm

8e9430c

Merge branch 'teal_refactor@main' of github.com:insightsengineering/t…

2260898

…eal.modules.clinical into teal_refactor@main

closes #668

f04850a

Fix styler

4c9531f

gogonzo mentioned this pull request Nov 4, 2022

Fix naming convention in the modules #669

Closed

gogonzo approved these changes Nov 4, 2022

View reviewed changes

gogonzo self-assigned this Nov 4, 2022

gogonzo merged commit f79d2c4 into main Nov 4, 2022

gogonzo deleted the teal_refactor@main branch November 4, 2022 16:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Teal refactor #598

Teal refactor #598

pawelru commented Sep 15, 2022 •

edited by nikolas-burkoff

Loading

pawelru Sep 15, 2022

gogonzo Nov 4, 2022

pawelru Sep 15, 2022

pawelru Sep 15, 2022

gogonzo Nov 4, 2022

pawelru Sep 15, 2022

gogonzo Nov 4, 2022

pawelru Sep 15, 2022

pawelru Sep 15, 2022

pawelru Sep 15, 2022

pawelru Sep 16, 2022

pawelru Sep 16, 2022

github-actions bot commented Oct 27, 2022 •

edited

Loading

gogonzo commented Nov 4, 2022

gogonzo commented Nov 4, 2022

gogonzo left a comment

Teal refactor #598

Teal refactor #598

Conversation

pawelru commented Sep 15, 2022 • edited by nikolas-burkoff Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 27, 2022 • edited Loading

Unit Tests Summary

gogonzo commented Nov 4, 2022

gogonzo commented Nov 4, 2022

gogonzo left a comment

Choose a reason for hiding this comment

pawelru commented Sep 15, 2022 •

edited by nikolas-burkoff

Loading

github-actions bot commented Oct 27, 2022 •

edited

Loading