feat(picks): migrate main (v2)#1470
Open
averissimo wants to merge 584 commits into
Open
Conversation
Contributor
|
@averissimo maybe change the base of the PR to redesign_extraction@main branch? |
Contributor
Author
|
@m7pr Last time I checked that branch was very old and had some code that we no longer use. It has since been updated, but it doesn't use the S3 dispatch method. We can cherry pick the code, but I think we should move away from that. Should we create a feature branch to use until |
Contributor
|
@averissimo I'll use this branch as the feature branch : ) |
m7pr
reviewed
Mar 16, 2026
m7pr
reviewed
Apr 2, 2026
Contributor
|
making it ready for a review, so we can run pipelines |
m7pr
marked this pull request as ready for review
April 2, 2026 07:38
m7pr
reviewed
Apr 2, 2026
Contributor
|
@averissimo if you have some time, any chance you can extend shinytest2 tests? |
Contributor
|
EDIT: fixed with vbumping teal version at the Remotes branch during insightsengineering/teal@4ee12ce Getting ℹ Creating lockfile '.github/pkg.lock'
ℹ Loading metadata database
✔ Loading metadata database ... done
ℹ Creating lockfile '.github/pkg.lock'
✖ Creating lockfile '.github/pkg.lock' [6.4s]
Error:
! error in pak subprocess
Caused by error:
! Could not solve package dependencies:
* deps::.: Can't install dependency insightsengineering/teal@redesign_extraction@main (>= 1.1.0.9029)
* any::sessioninfo: dependency conflict
---
Backtrace:
1. pak::lockfile_create(c(deps, extra_deps), lockfile = ".github/pkg.lock", …
2. pak:::remote(function(...) { …
3. err$throw(res$error)
---
Subprocess backtrace:
1. base::withCallingHandlers(cli_message = function(msg) { …
2. get("lockfile_create_internal", asNamespace("pak"))(...)
3. prop$stop_for_solution_error()
4. private$plan$stop_for_solve_error()
5. pkgdepends:::pkgplan_stop_for_solve_error(self, private)
6. base::throw(new_error("Could not solve package dependencies:\n", msg, …
7. | base::signalCondition(cond)
8. global (function (e) …
Execution halted
Error: Process completed with exit code 1. |
m7pr
reviewed
Apr 2, 2026
m7pr
reviewed
Apr 2, 2026
m7pr
reviewed
Apr 2, 2026
m7pr
reviewed
Apr 2, 2026
m7pr
reviewed
Apr 2, 2026
Contributor
Unit Tests Summary 1 files 74 suites 1h 53m 12s ⏱️ Results for commit b681610. ♻️ This comment has been updated with latest results. |
m7pr
reviewed
Apr 2, 2026
Contributor
Unit Test Performance DifferenceTest suite performance difference
Additional test case details
Results for commit b77135a ♻️ This comment has been updated with latest results. |
m7pr
reviewed
Apr 3, 2026
m7pr
reviewed
Apr 14, 2026
…nsightsengineering/teal.modules.clinical into 279-interactive_variables@main
# Pull Request It partially fixes #1472 Simple module with table. The code is not 100% reproducible yet, but is not specific to this module. E2E test have been fixed too. Use this app to test it: ``` devtools::load_all() data <- teal_data() data <- within(data, { ADSL <- tmc_ex_adsl ADCM <- tmc_ex_adcm }) join_keys(data) <- default_cdisc_join_keys[names(data)] adcm_keys <- c("STUDYID", "USUBJID", "ASTDTM", "CMSEQ", "ATC1", "ATC2", "ATC3", "ATC4") join_keys(data)["ADCM", "ADCM"] <- adcm_keys ADSL <- data[["ADSL"]] ADCM <- data[["ADCM"]] app <- init( data = data, modules = modules( tm_t_mult_events( label = "Concomitant Medications by Medication Class and Preferred Name", dataname = "ADCM", arm_var = variables(choices = c("ARM", "ARMCD"), selected = "ARM"), seq_var = variables(choices = "CMSEQ", selected = "CMSEQ", fixed = TRUE), hlt = variables( choices = c("ATC1", "ATC2", "ATC3", "ATC4"), selected = c("ATC1", "ATC2", "ATC3", "ATC4") ), llt = variables( choices = "CMDECOD", selected = "CMDECOD" ), add_total = TRUE, event_type = "treatment" ) ) ) if (interactive()) { shinyApp(app$ui, app$server) } ``` --------- Co-authored-by: Marcin <133694481+m7pr@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
averissimo
commented
May 14, 2026
| transformators = list(), | ||
| decorators = list()) { | ||
| message("Initializing tm_a_mmrm") | ||
| cov_var <- teal.transform::add_no_selected_choices(cov_var, multiple = TRUE) |
Contributor
Author
There was a problem hiding this comment.
Should we here add the "all-clear" = TRUE?
Contributor
There was a problem hiding this comment.
Still no idea what this "all-clear" does : P so can't help you : P
averissimo
commented
May 14, 2026
```
devtools::load_all('../teal.picks')
devtools::load_all('../teal')
devtools::load_all('.')
data <- teal_data()
data <- within(data, {
library(dplyr)
ADSL <- tmc_ex_adsl %>%
mutate(ITTFL = factor("Y") %>%
with_label("Intent-To-Treat Population Flag"))
ADAE <- tmc_ex_adae %>%
filter(!((AETOXGR == 1) & (AESEV == "MILD") & (ARM == "A: Drug X")))
})
join_keys(data) <- default_cdisc_join_keys[names(data)]
ADSL <- data[["ADSL"]]
ADAE <- data[["ADAE"]]
app <- init(
data = data,
modules = modules(
tm_g_barchart_simple(
label = "ADAE Analysis",
x = picks(
datasets("ADSL"),
variables(
choices = c(
"ARM", "ACTARM", "SEX",
"RACE", "ITTFL", "SAFFL", "STRATA2"
),
selected = "ACTARM",
multiple = FALSE
)
),
fill = picks(
datasets(choices = c("ADSL", "ADAE")),
variables(
choices = c(
"ARM", "ACTARM", "SEX",
"RACE", "ITTFL", "SAFFL", "STRATA2",
"AETOXGR", "AESEV", "AESER"
),
selected = "SEX",
multiple = FALSE
)
),
x_facet = picks(
datasets(choices = c("ADAE", "ADSL")),
variables(
choices = c(
"AETOXGR", "AESEV", "AESER",
"ARM", "ACTARM", "SEX",
"RACE", "ITTFL", "SAFFL", "STRATA2"
),
selected = "AETOXGR",
multiple = FALSE
)
),
y_facet = picks(
datasets(choices = c("ADAE", "ADSL")),
variables(
choices = c(
"AETOXGR", "AESEV", "AESER",
"ARM", "ACTARM", "SEX",
"RACE", "ITTFL", "SAFFL", "STRATA2"
),
selected = "AESEV",
multiple = FALSE
)
)
)
)
)
shiny::shinyApp(app$ui, app$server)
```
<img width="747" height="494" alt="image"
src="https://github.com/user-attachments/assets/6a0bb402-e588-4fc6-aad5-3b290da1e3ce"
/>
---------
Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com>
Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
averissimo
commented
May 15, 2026
Comment on lines
+364
to
+366
| worst_flag_indicator <- migrate_choices_selected_to_values(worst_flag_indicator, arg_name = "worst_flag_indicator") | ||
| worst_flag_value <- as.character(worst_flag_indicator$selected) | ||
| checkmate::assert_character(worst_flag_value, min.len = 1L, .var.name = "worst_flag_indicator$selected") |
Contributor
Author
Contributor
There was a problem hiding this comment.
It's probably becuase the default for worst_flag_indicator is set to
multiple = FALSEso that should be changed.
I wonder how we can detect that on all arguments in all modules : P ?
Contributor
There was a problem hiding this comment.
Hi, we had to actually change it as it was an html component and not an input. Here is the PR that changes this:
#1539
…me` removal (#1530) # Pull Request ### Changes description - `conf_values(_*)?` is simplified as only `fixed = TRUE|FALSE` should be used (it's not a real pick) - Removed "arg_name" parameter usage --------- Signed-off-by: André Veríssimo <211358+averissimo@users.noreply.github.com> Co-authored-by: Oriol Senan <35930244+osenan@users.noreply.github.com>
averissimo
commented
May 15, 2026
averissimo
commented
May 15, 2026
Fixes all checks in here #1527 except e2e tests. Please check the CI before merging --------- Signed-off-by: Oriol Senan <35930244+osenan@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
…nsightsengineering/teal.modules.clinical into 279-interactive_variables@main
# Pull Request ### Changes description - Add `teal.picks` prefix - Restore some of the previous defaults - Change `tm_t_tte` to single method --------- Signed-off-by: André Veríssimo <211358+averissimo@users.noreply.github.com> Co-authored-by: Oriol Senan <35930244+osenan@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: osenan <oriol.senan@businesspartner.roche.com>
It fixes #1527 E2E failing tests. One test was failing due to small differences between the validation message in browsers. The solution was a regexp instead of a fixed message for validation. Another test was failing due to the custom wait for JS not correctly capturing the table after some merges. Finally, the third module with failing tests was the tm_t_pp_laboratory. In all cases, the app had not loaded the table and the test were failing. I added a custom wait for js that checks that the table has rows. --------- Signed-off-by: Oriol Senan <35930244+osenan@users.noreply.github.com>
…gration (#1536) This are the necessary changes for this issue #1528 In module `tm_t_logistic.R` it was required to update the input validation. If not one argument could be NULL and input validation failed. In module `tm_t_exposure.R` the change was required on the documentation. The example did not included a value, which was necessary for the table to appear. Here the code for the `tm_t_exposure` module: ``` devtools::load_all() library(dplyr) data <- teal_data() data <- within(data, { ADSL <- tmc_ex_adsl ADEX <- tmc_ex_adex set.seed(1, kind = "Mersenne-Twister") .labels <- col_labels(ADEX, fill = FALSE) ADEX <- ADEX %>% distinct(USUBJID, .keep_all = TRUE) %>% mutate( PARAMCD = "TDURD", PARAM = "Overall duration (days)", AVAL = sample(x = seq(1, 200), size = n(), replace = TRUE), AVALU = "Days" ) %>% bind_rows(ADEX) col_labels(ADEX) <- .labels }) join_keys(data) <- default_cdisc_join_keys[names(data)] app <- init( data = data, modules = modules( tm_t_exposure( label = "Duration of Exposure Table", dataname = "ADEX", paramcd = variables(choices = "PARAMCD"), col_by_var = variables( choices = c("SEX", "ARM"), selected = "SEX" ), row_by_var = variables( choices = c("RACE", "REGION1", "STRATA1", "SEX"), selected = "RACE" ), parcat = picks(variables(choices = "PARCAT2"), values(), check_dataset = FALSE), add_total = FALSE ) ), filter = teal_slices(teal_slice("ADSL", "SAFFL", selected = "Y")) ) if (interactive()) { shinyApp(app$ui, app$server) } ``` Now for the tm_t_logistic module: ``` devtools::load_all() library(dplyr) data <- teal_data() data <- within(data, { ADSL <- tmc_ex_adsl ADRS <- tmc_ex_adrs %>% filter(PARAMCD %in% c("BESRSPI", "INVET")) }) join_keys(data) <- default_cdisc_join_keys[names(data)] ADSL <- data[["ADSL"]] ADRS <- data[["ADRS"]] 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") ) ) app <- init( data = data, modules = modules( tm_t_logistic( label = "Logistic Regression", dataname = "ADRS", paramcd = picks(variables("PARAMCD", "PARAMCD"), values(multiple = FALSE), check_dataset = FALSE ), cov_var = variables(selected = "ARM"), avalc_var = variables("AVALC", fixed = TRUE), conf_level = values(c(0.95, 0.9, 0.8), 0.95) ) ) ) if (interactive()) { shinyApp(app$ui, app$server) } ``` --------- Signed-off-by: Oriol Senan <35930244+osenan@users.noreply.github.com>
m7pr
reviewed
May 26, 2026
| @@ -0,0 +1,48 @@ | |||
| --- | |||
3 tasks
Closes #1537 The broad `observe` + `priority = 1L` was addressing a real bug (responders not updating when `arm/strata/merge` changed), but it was heavier than needed. I switched to `observeEvent(anl_q(), ...)`, which is the same pattern as `tm_t_events_patyear` / `tm_t_logistic` and matches the already-migrated `tm_g_forest_rsp` responders logic, without priority or `invisible()` dependencies. --------- Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com> Co-authored-by: Oriol Senan <35930244+osenan@users.noreply.github.com>
…ion (#1539) # Pull Request It fixes this comment [here](#1470 (comment)). We had slightly changed the behavior of the module after the migration. This PR keeps the use of `teal.picks::values` but allowing to use multiple worst flag indicators. Test it with the example on the module: ``` devtools::load_all() data <- teal_data() data <- within(data, { ADSL <- teal.modules.clinical::tmc_ex_adsl ADLB <- teal.modules.clinical::tmc_ex_adlb %>% dplyr::filter(!AVISIT %in% c("SCREENING", "BASELINE")) }) join_keys(data) <- default_cdisc_join_keys[names(data)] ADSL <- data[["ADSL"]] ADLB <- data[["ADLB"]] app <- init( data = data, modules = modules( tm_t_abnormality_by_worst_grade( label = "Laboratory Test Results with Highest Grade Post-Baseline", dataname = "ADLB", arm_var = choices_selected( choices = variable_choices(ADSL, subset = c("ARM", "ARMCD")), selected = "ARM" ), paramcd = choices_selected( choices = value_choices(ADLB, "PARAMCD", "PARAM"), selected = c("ALT", "CRP", "IGA") ), worst_flag_indicator = teal.transform::choices_selected(c("Y", "N")), add_total = FALSE ) ), filter = teal_slices( teal_slice("ADSL", "SAFFL", selected = "Y"), teal_slice("ADLB", "ONTRTFL", selected = "Y") ) ) if (interactive()) { shinyApp(app$ui, app$server) } ``` You should see the module working and two options available. Note: If you select "N" as worst flag indicator it will fail to compute the table. (It says it is computing and it never finishes). But it also happens in the example module before migration, that allowed multiple values and the "N" just does not work. It needs the right data that can allow multiple correct worst flag indicator. But the example proves that the multiple values are in the UI and used by the module. --------- Signed-off-by: André Veríssimo <211358+averissimo@users.noreply.github.com> Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.