Use picks in tm_g_spiderplot module

[osenan]

Partially closes issue #330

I also found a bug while testing the examples, if we have only one choice the UI of the picks component does not look ok.

The changes are:

Create method dispatch based on x_var. Refactor ui and server function to use picks instead of choices selected. I was able to remove some code of features handled by picks automatically like the merge. Maybe part of the validation should be implemented again, but I think I could remove it as well.

This app is a good test, it assumes you have teal.picks installed from latest main and are in this branch.

library("teal")
library("teal.picks")
devtools::load_all()

data <- teal_data() %>%
  within({
    library(nestcolor)
    ADSL <- teal.data::rADSL
    ADTR <- teal.data::rADTR
  })

join_keys(data) <- default_cdisc_join_keys[names(data)]

app <- init(
  data = data,
  modules = modules(
    tm_g_spiderplot(
      label = "Spider plot (picks)",
      dataname = "ADTR",
      paramcd = teal.picks::variables(
        choices = "PARAMCD",
        selected = "PARAMCD"
      ),
      x_var = teal.picks::variables(
        choices = dplyr::where(is.numeric),
        selected = 1L
      ),
      y_var = teal.picks::variables(
        choices = c("PCHG", "CHG", "AVAL"),
        selected = "PCHG"
      ),
      marker_var = teal.picks::variables(
        choices = c("SEX", "RACE", "USUBJID"),
        selected = "SEX"
      ),
      line_colorby_var = teal.picks::variables(
        choices = c("SEX", "USUBJID", "RACE"),
        selected = "SEX"
      ),
      xfacet_var = teal.picks::variables(
        choices = c("SEX", "ARM"),
        selected = "SEX"
      ),
      yfacet_var = teal.picks::variables(
        choices = c("SEX", "ARM"),
        selected = "ARM"
      ),
      vref_line = "10, 37",
      href_line = "-20, 0"
    ),
    tm_g_spiderplot(
      label = "Spider plot (default)",
      dataname = "ADTR",
      paramcd = choices_selected(
        choices = "PARAMCD",
        selected = "PARAMCD"
      ),
      x_var = choices_selected(
        choices = c("ADY", "AGE"),
        selected = "ADY"
      ),
      y_var = choices_selected(
        choices = c("PCHG", "CHG", "AVAL"),
        selected = "PCHG"
      ),
      marker_var = choices_selected(
        choices = c("SEX", "RACE", "USUBJID"),
        selected = "SEX"
      ),
      line_colorby_var = choices_selected(
        choices = c("SEX", "USUBJID", "RACE"),
        selected = "SEX"
      ),
      xfacet_var = choices_selected(
        choices = c("SEX", "ARM"),
        selected = "SEX"
      ),
      yfacet_var = choices_selected(
        choices = c("SEX", "ARM"),
        selected = "ARM"
      ),
      vref_line = "10, 37",
      href_line = "-20, 0"
    )
  )
)

shinyApp(app$ui, app$server)

[github-actions]

Code Coverage Summary

Filename                         Stmts    Miss  Cover    Missing
-----------------------------  -------  ------  -------  --------------------------------------------------------------
R/tm_g_ae_oview.R                  271     210  22.51%   90-94, 98-102, 169-402
R/tm_g_ae_sub.R                    271     237  12.55%   103-377
R/tm_g_butterfly.R                 360     316  12.22%   147, 150, 160-161, 200-561
R/tm_g_decorate.R                   42      42  0.00%    17-91
R/tm_g_events_term_id_picks.R      305     305  0.00%    23-406
R/tm_g_events_term_id.R            289     247  14.53%   131-407
R/tm_g_heat_bygrade.R              293     293  0.00%    136-459
R/tm_g_patient_profile.R           691     691  0.00%    159-913
R/tm_g_spiderplot.R                342     287  16.08%   188-524
R/tm_g_swimlane.R                  332     288  13.25%   65, 124-459
R/tm_g_waterfall.R                 414     374  9.66%    80-82, 141-569
R/utils_picks.R                    129      43  66.67%   18, 29-30, 70-74, 82-103, 129, 182, 185, 197-201, 223, 229-233
R/utils.R                           64      59  7.81%    31-73, 107-201
R/zzz.R                              3       3  0.00%    4-7
TOTAL                             3806    3395  10.80%

Diff against main

Filename                         Stmts    Miss  Cover
-----------------------------  -------  ------  --------
R/tm_g_ae_oview.R                  +46     -15  +22.51%
R/tm_g_ae_sub.R                    -17     -51  +12.55%
R/tm_g_butterfly.R                 -11     -55  +12.22%
R/tm_g_events_term_id_picks.R     +305    +305  +100.00%
R/tm_g_events_term_id.R            +18     -24  +14.53%
R/tm_g_spiderplot.R                +20     -35  +16.08%
R/tm_g_swimlane.R                  -13     -57  +13.25%
R/tm_g_waterfall.R                 -15     -55  +9.66%
R/utils_picks.R                   +129     +43  +66.67%
R/utils.R                          -21     -21  +1.93%
TOTAL                             +441     +35  +10.65%

Results for commit: 458d0dd

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

[github-actions]

Unit Test Performance Difference

Test Suite	$Status$	Time on main	$±Time$	$±Tests$	$±Skipped$	$±Failures$	$±Errors$
shinytest2-tm_g_events_term_id	👶		$+0.19$	$+5$	$+5$	$0$	$0$
tm_g_ae_oview	👶		$+0.17$	$+9$	$0$	$0$	$0$
tm_g_ae_sub	👶		$+0.08$	$+5$	$0$	$0$	$0$
tm_g_butterfly	👶		$+0.20$	$+11$	$0$	$0$	$0$
tm_g_events_term_id	👶		$+0.06$	$+8$	$0$	$0$	$0$
tm_g_spiderplot	👶		$+0.34$	$+15$	$0$	$+2$	$0$
tm_g_swimlane_waterfall_picks	👶		$+0.12$	$+14$	$0$	$0$	$0$

Additional test case details

Test Suite	$Status$	Time on main	$±Time$	Test Case
shinytest2-tm_g_events_term_id	👶		$+0.08$	e2e_tm_g_events_term_id_changing_term_var_changes_the_plot_and_does_not_throw_validation_errors.
shinytest2-tm_g_events_term_id	👶		$+0.02$	e2e_tm_g_events_term_id_deselection_of_term_var_throws_validation_error.
shinytest2-tm_g_events_term_id	👶		$+0.03$	e2e_tm_g_events_term_id_initializes_and_renders_a_plot
shinytest2-tm_g_events_term_id	👶		$+0.01$	e2e_tm_g_events_term_id_sort_updates_title
shinytest2-tm_g_events_term_id	👶		$+0.05$	e2e_tm_g_events_term_id_starts_with_expected_label_and_encoding_selections.
tm_g_ae_oview	👶		$+0.04$	tm_g_ae_oview_argument_verification_fails_when_arm_var_is_neither_picks_or_choices_selected
tm_g_ae_oview	👶		$+0.05$	tm_g_ae_oview_argument_verification_fails_when_flag_var_anl_is_neiter_picks_or_choices_selected
tm_g_ae_oview	👶		$+0.06$	tm_g_ae_oview_module_creation_creates_a_teal_module_using_choices_selected
tm_g_ae_oview	👶		$+0.02$	tm_g_ae_oview_module_creation_creates_a_teal_module_using_picks
tm_g_ae_sub	👶		$+0.06$	tm_g_ae_sub_input_validation_plot_arguments_input_validation
tm_g_ae_sub	👶		$+0.01$	tm_g_ae_sub_module_creation_creates_a_teal_module_using_choices_selected
tm_g_ae_sub	👶		$+0.01$	tm_g_ae_sub_module_creation_creates_a_teal_module_using_picks
tm_g_butterfly	👶		$+0.01$	tm_g_butterfly_input_validation_Forcing_Conversion_from_multiple_picks_to_single
tm_g_butterfly	👶		$+0.17$	tm_g_butterfly_input_validation_plot_arguments_input_validation
tm_g_butterfly	👶		$+0.01$	tm_g_butterfly_module_creation_creates_a_teal_module_using_choices_selected
tm_g_butterfly	👶		$+0.01$	tm_g_butterfly_module_creation_creates_a_teal_module_using_picks
tm_g_events_term_id	👶		$+0.04$	tm_g_events_term_id_builds_a_teal_module_with_picks_encodings
tm_g_events_term_id	👶		$+0.03$	tm_g_events_term_id_rejects_multiple_variable_selection
tm_g_spiderplot	👶		$+0.07$	tm_g_spiderplot_argument_verification_Forcing_Conversion_from_multiple_picks_to_single
tm_g_spiderplot	👶		$+0.23$	tm_g_spiderplot_argument_verification_plot_arguments_input_validation
tm_g_spiderplot	👶		$+0.03$	tm_g_spiderplot_module_creation_creates_a_teal_module_using_choices_selected_default_method_
tm_g_spiderplot	👶		$+0.01$	tm_g_spiderplot_module_creation_creates_a_teal_module_using_picks_.pick_method_
tm_g_swimlane_waterfall_picks	👶		$+0.08$	tm_g_swimlane_builds_a_teal_module_with_picks_encodings
tm_g_swimlane_waterfall_picks	👶		$+0.04$	tm_g_waterfall_builds_a_teal_module_with_picks_encodings

Results for commit d7bfa35

♻️ This comment has been updated with latest results.

[m7pr]

Right now this PR keeps both APIs

• choices_selected via tm_g_spiderplot.default
• as.picks plus tm_g_spiderplot.pick

which is a reasonable incremental step.

---

However I though we agreed that for modules without data_extract_spec
(see insightsengineering/teal.modules.clinical#1472 (comment))
we just go with a full migration like in teal.modules.clinical#1473 (e.g. tm_t_events, tm_t_summary).

I think we should align on how we move forward

[osenan]

I agree, since this are choices_selected we can convert and do not keep both APIs.

[osenan]

I have added more changes to the module. Note that the PR of butterfly module should be reviewed first.

The changes are:

• Keep defaults as in migration
• Remove S3 dispatch
• Remove teal.picks prefix on examples
• Update tests and documentation

[averissimo]

Functionality seems to work as expected 💯

I have some suggestions and topics of discussion, first on is: "Is butterfly wrongly included here?" 😅 I assumed so after running the example.

[osenan]

I try to implement all comments. In addition, I made change in the UI server to use formals and declare all arguments.
I will create an issue to join values and variables on a single picks.

[github-actions]

Unit Tests Summary

 1 files   8 suites   1s ⏱️
28 tests 22 ✅ 5 💤 1 ❌
80 runs  73 ✅ 5 💤 2 ❌

For more details on these failures, see this check.

Results for commit f6300d2.

♻️ This comment has been updated with latest results.

[osenan]

Thanks for the last review, I fixed branching issues and added very minor changes on documentation and a small refactor on how we check picks to be single

[averissimo]

Some minor comments regarding documentation and there's 1 comment left resolving from my previous review.

It should be ready to merge after these are addressed

[averissimo]

Please document all these arguments the other way round with teal.picks being referenced first as it is the preferred way.

[osenan]

Done, thanks

[averissimo]

Please document all these 2 arguments the other way round with teal.picks being referenced first as it is the preferred way.

[osenan]

Done, thanks

[averissimo]

[optional]

All of these variables overwrite the arguments with same name. This isn't ideal, but an improvement on this module would be to add a suffix _selected or _sel to better differentiate.

[osenan]

I will annotate this as a suggestion in the issue to unify patterns