Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Pre-release activities - vignettes review #640

Closed
m7pr opened this issue Feb 19, 2024 · 15 comments
Closed

Pre-release activities - vignettes review #640

m7pr opened this issue Feb 19, 2024 · 15 comments
Assignees
@kartikeyakirar
Labels
core enhancement New feature or request

Comments

@m7pr
Copy link
Contributor

m7pr commented Feb 19, 2024 •
edited
Loading

Part of #624 related to vignettes. Please make a PR to pre-release@main
@m7pr m7pr added enhancement New feature or request core labels Feb 19, 2024
@m7pr m7pr mentioned this issue Feb 19, 2024
Closed
26 tasks
@kartikeyakirar kartikeyakirar self-assigned this Feb 22, 2024
@kartikeyakirar kartikeyakirar mentioned this issue Feb 26, 2024
Merged
@chlebowa
Copy link
Contributor

The example apps in the vignettes are quite verbose, e.g.

very long example 
# nolint start
data <- teal_data()
data <- within(data, {
  ADSL <- teal.modules.general::rADSL
  ADSL2 <- ADSL %>%
    mutate(TRTDUR = round(as.numeric(TRTEDTM - TRTSDTM), 1))
  ADRS <- teal.modules.general::rADRS
  ADTTE <- teal.modules.general::rADTTE
  ADLB <- teal.modules.general::rADLB %>%
    mutate(CHGC = as.factor(case_when(
      CHG < 1 ~ "N",
      CHG > 1 ~ "P",
      TRUE ~ "-"
    )))
})
datanames <- c("ADSL", "ADSL2", "ADRS", "ADTTE", "ADLB")
datanames(data) <- datanames
jk <- default_cdisc_join_keys[datanames]
jk_adsl2 <- jk
names(jk_adsl2)[names(jk_adsl2) == "ADSL"] <- "ADSL2"
jk <- c(jk, jk_adsl2)
jk["ADSL2", "ADSL"] <- c("USUBJID", "STUDYID")

# nolint end

app <- teal::init(
  data = data,
  modules = teal::modules(
    # tm_g_association ----
    modules(
      label = "Association plot",
      tm_g_association(
        label = "Single wide dataset",
        ref = teal.transform::data_extract_spec(
          dataname = "ADSL",
          select = select_spec(
            label = "Select variable:",
            choices = variable_choices(data[["ADSL"]]),
            selected = "AGE",
            fixed = FALSE
          )
        ),
        vars = teal.transform::data_extract_spec(
          dataname = "ADSL",
          select = select_spec(
            label = "Select variables:",
            choices = variable_choices(data[["ADSL"]]),
            selected = "BMRKR1",
            multiple = TRUE,
            fixed = FALSE
          )
        )
      ),
      tm_g_association(
        label = "Two wide datasets",
        ref = teal.transform::data_extract_spec(
          dataname = "ADSL",
          select = select_spec(
            label = "Select variable:",
            choices = variable_choices(data[["ADSL"]], c("AGE", "SEX", "STRATA1", "RACE")),
            selected = "STRATA1",
            multiple = FALSE,
            fixed = FALSE
          )
        ),
        vars = teal.transform::data_extract_spec(
          dataname = "ADSL2",
          select = select_spec(
            label = "Select variables:",
            choices = variable_choices(data[["ADSL2"]], c("AGE", "SEX", "RACE", "COUNTRY")),
            selected = c("AGE", "COUNTRY", "RACE"),
            multiple = TRUE,
            fixed = FALSE
          )
        )
      ),
      tm_g_association(
        label = "Multiple different long datasets",
        ref = teal.transform::data_extract_spec(
          dataname = "ADTTE",
          select = select_spec(
            label = "Select variables:",
            choices = variable_choices(data[["ADTTE"]]),
            selected = "AVAL",
            multiple = FALSE,
            fixed = FALSE
          ),
          filter = teal.transform::filter_spec(
            label = "Select endpoint:",
            vars = "PARAMCD",
            choices = value_choices(data[["ADTTE"]], "PARAMCD", "PARAM"),
            selected = c("PFS", "EFS"),
            multiple = TRUE
          )
        ),
        vars = teal.transform::data_extract_spec(
          dataname = "ADRS",
          reshape = TRUE,
          select = select_spec(
            label = "Select variable:",
            choices = variable_choices(data[["ADRS"]], c("AVALC", "BMRKR1", "BMRKR2", "ARM")),
            selected = "AVALC",
            multiple = TRUE,
            fixed = FALSE
          ),
          filter = list(
            filter_spec(
              label = "Select endpoints:",
              vars = "PARAMCD",
              choices = value_choices(data[["ADRS"]], "PARAMCD", "PARAM"),
              selected = "BESRSPI",
              multiple = TRUE
            ),
            filter_spec(
              label = "Select endpoints:",
              vars = "AVISIT",
              choices = levels(data[["ADRS"]]$AVISIT),
              selected = "SCREENING",
              multiple = TRUE
            )
          )
        )
      ),
      tm_g_association(
        label = "Wide and long datasets",
        ref = teal.transform::data_extract_spec(
          dataname = "ADRS",
          select = select_spec(
            choices = variable_choices(data[["ADRS"]], c("AVAL", "AVALC")),
            selected = "AVALC",
            multiple = FALSE,
            fixed = FALSE,
            label = "Selected variable:"
          ),
          filter = list(
            filter_spec(
              vars = "PARAMCD",
              choices = value_choices(data[["ADRS"]], "PARAMCD", "PARAM"),
              selected = levels(data[["ADRS"]]$PARAMCD),
              multiple = TRUE,
              label = "Select response"
            ),
            filter_spec(
              vars = "AVISIT",
              choices = levels(data[["ADRS"]]$AVISIT),
              selected = levels(data[["ADRS"]]$AVISIT),
              multiple = TRUE,
              label = "Select visit:"
            )
          )
        ),
        vars = teal.transform::data_extract_spec(
          dataname = "ADSL",
          select = select_spec(
            choices = variable_choices(data[["ADSL"]], c("SEX", "AGE", "RACE", "COUNTRY", "BMRKR1", "STRATA1", "ARM")),
            selected = "AGE",
            multiple = TRUE,
            fixed = FALSE,
            label = "Select variable:"
          )
        )
      ),
      tm_g_association(
        label = "Same long datasets (same subsets)",
        ref = teal.transform::data_extract_spec(
          dataname = "ADRS",
          select = select_spec(
            choices = variable_choices(data[["ADRS"]]),
            selected = "AVALC",
            multiple = FALSE,
            fixed = FALSE,
            label = "Select variable:"
          )
        ),
        vars = teal.transform::data_extract_spec(
          dataname = "ADRS",
          select = select_spec(
            choices = variable_choices(data[["ADRS"]]),
            selected = "PARAMCD",
            multiple = TRUE,
            fixed = FALSE,
            label = "Select variable:"
          )
        )
      ),
      tm_g_association(
        label = "Same long datasets (different subsets)",
        ref = teal.transform::data_extract_spec(
          dataname = "ADLB",
          filter = list(
            filter_spec(
              vars = "PARAMCD",
              choices = value_choices(data[["ADLB"]], "PARAMCD", "PARAM"),
              selected = levels(data[["ADLB"]]$PARAMCD)[1],
              multiple = FALSE,
              label = "Select lab:"
            ),
            filter_spec(
              vars = "AVISIT",
              choices = levels(data[["ADLB"]]$AVISIT),
              selected = levels(data[["ADLB"]]$AVISIT)[1],
              multiple = FALSE,
              label = "Select visit:"
            )
          ),
          select = select_spec(
            choices = variable_choices(data[["ADLB"]], c("AVAL", "CHG2", "PCHG2")),
            selected = "AVAL",
            multiple = FALSE
          )
        ),
        vars = teal.transform::data_extract_spec(
          dataname = "ADLB",
          filter = list(
            filter_spec(
              vars = "PARAMCD",
              choices = value_choices(data[["ADLB"]], "PARAMCD", "PARAM"),
              selected = levels(data[["ADLB"]]$PARAMCD)[1],
              multiple = FALSE,
              label = "Select labs:"
            ),
            filter_spec(
              vars = "AVISIT",
              choices = levels(data[["ADLB"]]$AVISIT),
              selected = levels(data[["ADLB"]]$AVISIT)[1],
              multiple = FALSE,
              label = "Select visit:"
            )
          ),
          select = select_spec(
            choices = variable_choices(data[["ADLB"]]),
            selected = "STRATA1",
            multiple = TRUE
          )
        )
      )
    )
  )
)

What do you think about assigning modules as separate variables to make the init call more concise?

mod1 <- tm_g_association(label = "Single wide dataset" [...])
mod2 <- tm_g_association(label = "Two wide datasets" [...])

[...]

app <- init(
  data = data,
  modules = modules(
    # tm_g_association ----
    modules(
      label = "Association plot",
      mod1,
      mod2
[...]

@m7pr
Copy link
Contributor Author

m7pr commented Feb 26, 2024

Yes, this makes a huge sense! +1

@chlebowa
Copy link
Contributor

Another thought: do you think it would be beneficial to add options = list("launch.browser" = TRUE) to running the apps. Alternatively use runApp(app, launch.browser = TRUE)?

@m7pr
Copy link
Contributor Author

m7pr commented Feb 26, 2024

I think runApp(app, launch.browser = TRUE) is cleaner. Good idea

@vedhav
Copy link
Contributor

vedhav commented Feb 26, 2024

When running the example app from the vignette using-association-plot.Rmd (This is also noticed in examples from other vignettes too) Possibly due to incorrect data spec as @gogonzo mentioned.
There are a few modules that do not show output and display this error:

Cannot merge at least two dataset extracts. Make sure all datasets used for merging have appropriate keys.
Screenshot 2024-02-26 at 8 45 18 PM

@gogonzo
Copy link
Contributor

gogonzo commented Feb 26, 2024

When running the example app from the vignette using-association-plot.Rmd (This is also noticed in examples from other vignettes too) Possibly due to incorrect data spec as @gogonzo mentioned. There are a few modules that do not show output and display this error:

Cannot merge at least two dataset extracts. Make sure all datasets used for merging have appropriate keys.
Screenshot 2024-02-26 at 8 45 18 PM

Definitely data-extract/data-merge problem. We have something to debug here

@vedhav
Copy link
Contributor

vedhav commented Feb 26, 2024

Changing the ADSL2 to ADSL fixes this. Looks like some parent-child relationship mapping is causing it.

@vedhav vedhav mentioned this issue Feb 27, 2024
Merged
@vedhav
Copy link
Contributor

vedhav commented Feb 27, 2024

🤦🏽 The created join_keys were not being bounded to the teal_data object so the example app was not working.

vedhav added a commit that referenced this issue Feb 27, 2024
@vedhav
Vignette changes (#687)
1fae848 
Closes #640

---------

Signed-off-by: Vedha Viyash <[email protected]>
@gogonzo
Copy link
Contributor

gogonzo commented Feb 27, 2024

Changing the ADSL2 to ADSL fixes this. Looks like some parent-child relationship mapping is causing it.

I think we can remove ADSL2 and create TRTDUR variable in ADSL instead of creating a new dataset. I think that back then it was only to demonstrate merge between datasets with the same join keys. I think everybody agree that ADSL2 makes unnecessary complications.

@vedhav
Copy link
Contributor

vedhav commented Feb 27, 2024

Changing the ADSL2 to ADSL fixes this. Looks like some parent-child relationship mapping is causing it.

I think we can remove ADSL2 and create TRTDUR variable in ADSL instead of creating a new dataset. I think that back then it was only to demonstrate merge between datasets with the same join keys. I think everybody agree that ADSL2 makes unnecessary complications.

I agree, I don't see much value in having ADSL2 here and the teal.data vignettes explain join_keys in detail. @kartikeyakirar could you please remove ADSL2 from your PR #681?

kartikeyakirar added a commit that referenced this issue Feb 28, 2024
@kartikeyakirar @vedhav
pre-release vignettes review (#681)
804d79a 
PR to
#640

---------

Signed-off-by: Vedha Viyash <[email protected]>
Co-authored-by: kartikeya <[email protected]>
Co-authored-by: Vedha Viyash <[email protected]>
@m7pr
Copy link
Contributor Author

m7pr commented Feb 28, 2024

Hey @kartikeyakirar did you mind looking at this comment
#640 (comment)
and this one #640 (comment)
when merging/doing the review of vignettes?

@kartikeyakirar
Copy link
Contributor

kartikeyakirar commented Feb 28, 2024 •
edited
Loading

@m7pr I assigned modules to separate variables for a more concise init call, but I missed to add launch.browser = TRUE. Thank you for pointing it out. Would you mind if I add it to the pre-release branch?

@kartikeyakirar
Copy link
Contributor

Another thought: do you think it would be beneficial to add options = list("launch.browser" = TRUE) to running the apps. Alternatively use runApp(app, launch.browser = TRUE)?

@m7pr @chlebowa sorry I missed this part but do we want to change all shinyApp() calls torunApp()?

@chlebowa
Copy link
Contributor

Well, that's up for discussion. I think vignettes definitely benefit from this, so there at least. I myself run example apps for modules in the browser as well (but I don't want to set it as IDE default because I still want small apps to launch in the RStudio panel) so it would work for me but not necessarily for others.

@kartikeyakirar kartikeyakirar mentioned this issue Feb 29, 2024
Open
3 tasks
@kartikeyakirar
Copy link
Contributor

I added a separate task for discussion. This needs to be applied to all package vignettes for consistency. insightsengineering/nestdevs-tasks#53

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kartikeyakirar kartikeyakirar

Labels
core enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@kartikeyakirar @gogonzo @vedhav @chlebowa @m7pr