init reporter tmc #518

Polkas · 2022-06-24T13:45:08Z

closes #470

depends on insightsengineering/teal.reporter#84 which have to be merge first.

Introduce the teal.reporter functionality to the 5 tmc modules.

follow the guidelines from #505

Critical Encoding is not added to any of the modules.
All important encoding seems to be already part of modules table/plot

MMRM module has internal tabPanel with separated elements, plots and tables.
MMRM reporter card could catch only one plot/table at a time (when add card button is clicked).
This comes from the design of the MMRM module, each module plot/table is evaluated separately and have own Show R code. The MMRM card could be added only after the regression model is evaluated.

the with_reporter if condition is a preparation for the future independence of teal modules.

New Issues:

Example:

all 5 modules plus one WITHOUT the reporter (tm_t_coxreg).

library(scda)

ADSL <- synthetic_cdisc_data("latest")$adsl
ADTTE <- synthetic_cdisc_data("latest")$adtte
ADAE <- synthetic_cdisc_data("latest")$adae
ADQS <- synthetic_cdisc_data("latest")$adqs %>%
  dplyr::filter(ABLFL != "Y" & ABLFL2 != "Y") %>%
  dplyr::filter(AVISIT %in% c("WEEK 1 DAY 8", "WEEK 2 DAY 15", "WEEK 3 DAY 22")) %>%
  dplyr::mutate(
    AVISIT = as.factor(AVISIT),
    AVISITN = rank(AVISITN) %>%
      as.factor() %>%
      as.numeric() %>%
      as.factor() # making consecutive numeric factor
  )

arm_ref_comp <- list(
  ACTARMCD = list(
    ref = "ARM B",
    comp = c("ARM A", "ARM C")
  ),
  ARM = list(
    ref = "B: Placebo",
    comp = c("A: Drug X", "C: Combination")
  ),
  ARMCD = list(
    ref = "ARM B",
    comp = c("ARM A", "ARM C")
  )
)

app <- init(
  data = cdisc_data(
    cdisc_dataset("ADSL", ADSL, code = 'ADSL <- synthetic_cdisc_data("latest")$adsl'),
    cdisc_dataset("ADAE", ADAE, code = 'ADAE <- synthetic_cdisc_data("latest")$adae'),
    cdisc_dataset("ADTTE", ADTTE, code = 'ADTTE <- synthetic_cdisc_data("latest")$adtte'),
    cdisc_dataset("ADQS", ADQS,
                  code = 'ADQS <- synthetic_cdisc_data("latest")$adqs %>%
              dplyr::filter(ABLFL != "Y" & ABLFL2 != "Y") %>%
              dplyr::filter(AVISIT %in% c("WEEK 1 DAY 8", "WEEK 2 DAY 15", "WEEK 3 DAY 22")) %>%
              dplyr::mutate(
                AVISIT = as.factor(AVISIT),
                AVISITN = rank(AVISITN) %>%
                  as.factor() %>%
                  as.numeric() %>%
                  as.factor() # making consecutive numeric factor
              )'
    ),
    check = TRUE
  ),
  modules = modules(
    tm_g_km(
      label = "KM PLOT",
      dataname = "ADTTE",
      arm_var = choices_selected(
        variable_choices(ADSL, c("ARM", "ARMCD", "ACTARMCD")),
        "ARM"
      ),
      paramcd = choices_selected(
        value_choices(ADTTE, "PARAMCD", "PARAM"),
        "OS"
      ),
      arm_ref_comp = arm_ref_comp,
      strata_var = choices_selected(
        variable_choices(ADSL, c("SEX", "BMRKR2")),
        "SEX"
      ),
      facet_var = choices_selected(
        variable_choices(ADSL, c("SEX", "BMRKR2")),
        NULL
      )
    ),
    tm_t_summary(
      label = "Demographic Table",
      dataname = "ADSL",
      arm_var = choices_selected(c("ARM", "ARMCD"), "ARM"),
      add_total = TRUE,
      summarize_vars = choices_selected(
        c("SEX", "RACE", "BMRKR2", "EOSDY", "DCSREAS", "AGE"),
        c("SEX", "RACE")
      ),
      useNA = "ifany"
    ),
    tm_a_mmrm(
      label = "MMRM",
      dataname = "ADQS",
      aval_var = choices_selected(c("AVAL", "CHG"), "AVAL"),
      id_var = choices_selected(c("USUBJID", "SUBJID"), "USUBJID"),
      arm_var = choices_selected(c("ARM", "ARMCD"), "ARM"),
      visit_var = choices_selected(c("AVISIT", "AVISITN"), "AVISIT"),
      arm_ref_comp = arm_ref_comp,
      paramcd = choices_selected(
        choices = value_choices(ADQS, "PARAMCD", "PARAM"),
        selected = "FKSI-FWB"
      ),
      cov_var = choices_selected(c("BASE", "AGE", "SEX", "BASE:AVISIT"), NULL)
    ),
    tm_t_events(
      label = "Adverse Event Table",
      dataname = "ADAE",
      arm_var = choices_selected(c("ARM", "ARMCD"), "ARM"),
      llt = choices_selected(
        choices = variable_choices(ADAE, c("AETERM", "AEDECOD")),
        selected = c("AEDECOD")
      ),
      hlt = choices_selected(
        choices = variable_choices(ADAE, c("AEBODSYS", "AESOC")),
        selected = "AEBODSYS"
      ),
      add_total = TRUE,
      event_type = "adverse event"
    ),
    tm_t_tte(
      label = "Time To Event Table",
      dataname = "ADTTE",
      arm_var = choices_selected(
        variable_choices(ADSL, c("ARM", "ARMCD", "ACTARMCD")),
        "ARM"
      ),
      arm_ref_comp = arm_ref_comp,
      paramcd = choices_selected(
        value_choices(ADTTE, "PARAMCD", "PARAM"),
        "OS"
      ),
      strata_var = choices_selected(
        variable_choices(ADSL, c("SEX", "BMRKR2")),
        "SEX"
      ),
      time_points = choices_selected(c(182, 243), 182),
      event_desc_var = choices_selected(
        variable_choices(ADTTE, "EVNTDESC"),
        "EVNTDESC",
        fixed = TRUE
      )
    ),
    tm_t_coxreg(
      label = "Cox Reg.",
      dataname = "ADTTE",
      arm_var = choices_selected(c("ARM", "ARMCD", "ACTARMCD"), "ARM"),
      arm_ref_comp = arm_ref_comp,
      paramcd = choices_selected(
        value_choices(ADTTE, "PARAMCD", "PARAM"), "OS"
      ),
      strata_var = choices_selected(
        c("COUNTRY", "STRATA1", "STRATA2"), "STRATA1"
      ),
      cov_var = choices_selected(
        c("AGE", "BMRKR1", "BMRKR2", "REGION1"), "AGE"
      ),
      multivariate = TRUE
    )
  )
)

shinyApp(ui = app$ui, server = app$server)

github-actions · 2022-06-27T15:01:35Z

Unit Tests Summary

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

Results for commit 6d4f942.

♻️ This comment has been updated with latest results.

nikolas-burkoff · 2022-06-28T15:02:25Z

@shajoezhu @danielinteractive I'm taking a look at this from a code perspective but I think you guys (or someone in your teams) should have a look as well -> Note the new issues at the top of the PR that @Polkas has already created

nikolas-burkoff · 2022-06-28T15:10:10Z

R/tm_t_tte.R

+        card$set_name("Time To Event Table")
+        card$append_text("Time To Event Table", "header2")
+        card$append_text("Filter State", "header3")
+        card$append_fs(datasets)


@gogonzo are there implication here using datasets during the teal refactor? Though we will need the state of the filter panel for the report

we need not only state, we are need the format method. That is why we need to pass here datasets as a whole. This was surprising for me and @mhallal1 to have this FilterData argument here , still we not oppose to it at that moment.

yeah it's fine for now, I imagine once the state of the filter panel is decoupled from the data then this will need to be changed - but that's ok

This will be affected once we decide to push data instead of datasets. This means that we will probably $add_text and $add_metadata directly.

https://github.com/insightsengineering/teal.reporter/blob/66ebb849bc7b8c67412fa1a75a19207637c7f361/R/TealReportCard.R#L44

nikolas-burkoff · 2022-06-28T15:11:17Z

R/tm_t_tte.R

+        }
+        card$append_text("Show R Code", "header3")
+        card$append_src(paste(get_rcode(
+          chunks = session$userData[[session$ns(character(0))]]$chunks,


I think there's a function teal.code::get_chunks_object() to get this?

I tried before the proposed solution and did not work.

yeah that should work - I am quite surprised that it's not. Can you give another try?

teal.code::get_chunks_object() vs session$userData[[session$ns(character(0))]]$chunks

Error in get_rcode(chunks = teal.code::get_chunks_object(), datasets = datasets, : No code chunks given

I think I know why teal.code::get_chunks_object() not works. The session$ns is incorrect in the add card module as uses the local session$ns not the one from the tm_module_srv body.

Alternative is usage of chunks = teal.code::get_chunks_object(parent_idx = 1L)

@nikolas-burkoff thanks for the proposal. just update it.

nikolas-burkoff · 2022-06-28T15:15:51Z

R/tm_a_mmrm.R

+        if (!is.null(mmrm_table())) {
+          card$append_table(mmrm_table())
+        }
+        if (!is.null(mmrm_plot_reactive())) {
+          card$append_plot(mmrm_plot_reactive())
+        }


I wonder if we should have the name of the plot/table i.e. the value of the radio button also included in the report)

I can imagine the method used to fit the model - could be useful information from the encoding panel in the report?

What happens if you add to report before you've fitted the model?

We could change the subtitle dynamically.

You mean the Optimization Algorithm? we could add it too.

Card WIll not be added. App is NOT crashed.

The only place I feel encoding could be valuable is MMRM, still in the first table visible after estimation we see most of the important parameters.

Card WIll not be added. App is NOT crashed.

True but you click the Add card button in the modal and nothing happens (i.e. there's no feedback to the user as to what is going on and they have to click cancel)

The only place I feel encoding could be valuable is MMRM, still in the first table visible after estimation we see most of the important parameters.

I'm not sure about that - for example in the KM plot in the example we have these fixed:
but they could be configurable? And their output is not on the plot. Similarly we are stratifying by SEX by default that is not included and can be changed

And also these are probably useful:

I could investigate it tomorrow. For sure is not a part of this PR more a candidate for an issue in the teal.reporter.

stratification, comparison settings would be nice to be added as footnotes as those are non-critical enough to be on the subtitles session

The redundant click in the MMRM is solved here insightsengineering/teal.reporter#87
I added the try clause and print a proper message if the process failed.

Thanks all for work on the MMRM module, please check with me for any larger scale changes on functionality etc.

Also I am wondering if it would be better to split out the MMRM module mid-term (few months) in a separate package. Since then we could split this off the main NEST release and have it shipped separately with mmrm, tern.mmrm, teal.modules.mmrm outside of NEST - no longer limited by infrastructure problems etc.
@pawelru fyi re: our discussion today.

Yeah it sounds good to split things up sensibly - my only concern would be as the number of packages we've created grows - we need a proper support model for maintaining/updating/releasing them all - even for trivial fixes - @dinakar29 @pawelru and I'm not clear we have one yet - e.g. internal users will still need these packages available internally (not on old infrastructure of course) and things will break and/or become inconsistent as newer versions of R/R packages are released.

maintaining - better integration tests
updating/releasing - simpler release model

I have chatted a little with Dinakar and I might have an idea for the latter one. But this goes way beyond that PR so let's close this topic not to destroy the review.

nikolas-burkoff

So from a code and functionality perspective this looks really good.

I suspect there will be more things needed in the cards (possibly in footnotes) - @kumamiao @insightsengineering/nest-sme will need to specify what -> Joe and Nina should decide how we are going to do that review - i.e. it could be that we merge this in and there's a separate issue for Nina/SME to review all the tmc modules to request what else is added or it could be done for these modules before merging in.

Not for this PR but:

I also suspect there will need to be some changes to what happens with the show R code, I suspect a lot of users will not want a whole huge block of R code between each output when rendering to say pdf/html especially if there's a few hundred lines of pre-processing included in every report card - I can think of a few different approaches:
- have an option to add echo = FALSE to the R Code markdown chunk
- only output the code for the module in the card and put the pre-processing code in an appendix/at the beginning of the report
- have an option when creating the report card whether to include the show r code
- move all the show r code to an appendix
I think there could be other really nice to have bits of functionality users may want which we should capture in the backlog - the sprint review could be a really good time to show people and to get feedback to make sure we make the reporter as useful as possible

Thoughts @kumamiao ?

pawelru · 2022-06-29T09:40:10Z

R/tm_t_tte.R

+      }
+
+      teal.reporter::add_card_button_srv("addReportCard", reporter = reporter, card_fun = card_fun)
+      teal.reporter::download_report_button_srv("downloadButton", reporter = reporter)


I know that it's not for tmc but rather teal.reporter itself but let me comment that as this is the very first time we can see it in action.
I don't yet feel the need of download from the module window. It's like for online shopping - you add items to your cart and then see the summary and proceed to the checkout. I wonder if removing this could simplify the user journey as well as implementation.

we assume to leave the Download/Reset buttons there from the beginning . This gives more flexible journey for an ad-hoc analysis, when sb does not have time to jump into the Previewer. Moreover we assume that more options is better than less.
@kpagacz

so if we want we can remove the download button here and in the UI for modules without needing to change teal.reporter I think

yes, it is as easy as to remove 2 lines of code in the UI and server each (for each tmc module). Still I am not recommending it.

R/tm_t_summary.R

Polkas · 2022-06-30T09:33:52Z

The PR is now unblocked:)

gogonzo · 2022-07-01T13:09:07Z

I checked example app and run efficacy also. They fail after clicking "add card" (second "add card" after editing comment)

nikolas-burkoff · 2022-07-01T13:45:06Z

I checked example app and run efficacy also. They fail after clicking "add card" (second "add card" after editing comment)

The example at the top of the PR which previously worked for me now fails as well so something has changed somewhere...

nikolas-burkoff · 2022-07-01T13:53:51Z

ah I needed to merge main into insightsengineering/teal.reporter#84 so it should be all good now @gogonzo

gogonzo

Everything works as expected. No risk of breaking anything as change only adds stuff. We can talk later about the style of the buttons

init reporter tmc

9ab1725

Polkas added the core label Jun 24, 2022

Polkas added 5 commits June 24, 2022 16:46

km

9e1de3d

other modules

0605730

NEWS

22a2489

Merge branch 'main' into 470_reporter5@main

703e58f

no encoding

59f4951

Polkas marked this pull request as ready for review June 27, 2022 14:51

Polkas added 3 commits June 27, 2022 17:12

styler + deps

d458d9d

stage

0aeb7ca

stage

1175197

Polkas mentioned this pull request Jun 27, 2022

Update staged_dependencies.yaml insightsengineering/teal.reporter#84

Merged

Polkas added sme blocked and removed blocked labels Jun 27, 2022

Polkas added 2 commits June 28, 2022 13:18

with_reporter

3c844d8

styler

a4b2bbd

Polkas mentioned this pull request Jun 28, 2022

Refactor MMRM module - combined outputs - Reporter #519

Closed

Polkas requested review from a team June 28, 2022 11:52

nikolas-burkoff self-assigned this Jun 28, 2022

nikolas-burkoff reviewed Jun 28, 2022

View reviewed changes

nikolas-burkoff reviewed Jun 29, 2022

View reviewed changes

pawelru mentioned this pull request Jun 29, 2022

Refactor filter panel - summary and collapsible insightsengineering/teal.slice#55

Closed

pawelru reviewed Jun 29, 2022

View reviewed changes

R/tm_t_summary.R Outdated Show resolved Hide resolved

Polkas added the blocked label Jun 29, 2022

Polkas added 2 commits June 29, 2022 16:25

Merge branch 'main' into 470_reporter5@main

3c9395e

teal.code::get_chunks_object

8da54c5

nikolas-burkoff mentioned this pull request Jun 29, 2022

Visualization settings: capturing provenance: filters and encodings insightsengineering/teal#549

Closed

Polkas added 2 commits June 29, 2022 20:11

styler

c4c2169

new init of the card

4bd34c9

Polkas removed the blocked label Jun 30, 2022

Merge branch 'main' into 470_reporter5@main

6d4f942

gogonzo self-assigned this Jul 1, 2022

Polkas mentioned this pull request Jul 1, 2022

Disappearing filter summary insightsengineering/teal.slice#59

Closed

gogonzo approved these changes Jul 1, 2022

View reviewed changes

Polkas merged commit a410508 into main Jul 1, 2022

Polkas deleted the 470_reporter5@main branch July 1, 2022 14:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

init reporter tmc #518

init reporter tmc #518

Polkas commented Jun 24, 2022 •

edited

Loading

github-actions bot commented Jun 27, 2022 •

edited

Loading

nikolas-burkoff commented Jun 28, 2022

nikolas-burkoff Jun 28, 2022

Polkas Jun 28, 2022 •

edited

Loading

nikolas-burkoff Jun 28, 2022

gogonzo Jul 1, 2022

nikolas-burkoff Jun 28, 2022

Polkas Jun 28, 2022

nikolas-burkoff Jun 28, 2022

pawelru Jun 29, 2022

Polkas Jun 29, 2022

Polkas Jun 29, 2022

Polkas Jun 29, 2022 •

edited

Loading

nikolas-burkoff Jun 28, 2022

Polkas Jun 28, 2022 •

edited

Loading

nikolas-burkoff Jun 28, 2022

nikolas-burkoff Jun 28, 2022

Polkas Jun 28, 2022

kumamiao Jun 28, 2022

Polkas Jun 28, 2022

danielinteractive Jun 28, 2022

nikolas-burkoff Jun 29, 2022

pawelru Jun 29, 2022

nikolas-burkoff left a comment

pawelru Jun 29, 2022

Polkas Jun 29, 2022 •

edited

Loading

nikolas-burkoff Jun 29, 2022

Polkas Jun 29, 2022 •

edited

Loading

Polkas commented Jun 30, 2022

gogonzo commented Jul 1, 2022 •

edited

Loading

nikolas-burkoff commented Jul 1, 2022

nikolas-burkoff commented Jul 1, 2022

gogonzo left a comment

init reporter tmc #518

init reporter tmc #518

Conversation

Polkas commented Jun 24, 2022 • edited Loading

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

Unit Tests Summary

nikolas-burkoff commented Jun 28, 2022

Choose a reason for hiding this comment

Polkas Jun 28, 2022 • edited 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

Polkas Jun 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Polkas Jun 28, 2022 • edited 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

nikolas-burkoff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Polkas Jun 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Polkas Jun 29, 2022 • edited Loading

Choose a reason for hiding this comment

Polkas commented Jun 30, 2022

gogonzo commented Jul 1, 2022 • edited Loading

nikolas-burkoff commented Jul 1, 2022

nikolas-burkoff commented Jul 1, 2022

gogonzo left a comment

Choose a reason for hiding this comment

Polkas commented Jun 24, 2022 •

edited

Loading

github-actions bot commented Jun 27, 2022 •

edited

Loading

Polkas Jun 28, 2022 •

edited

Loading

Polkas Jun 29, 2022 •

edited

Loading

Polkas Jun 28, 2022 •

edited

Loading

Polkas Jun 29, 2022 •

edited

Loading

Polkas Jun 29, 2022 •

edited

Loading

gogonzo commented Jul 1, 2022 •

edited

Loading