tm_t_summary* hide NAs when useNA = "no"

[llrs-roche]

Pull Request

Fixes #1437

• Cleans up a bit the code (removed some duplicated expression and reorganize a bit the code).
• df_explicit_na() expression is only added when useNA = "ifany".

Check also with tern issue: insightsengineering/tern#1440

To check run the app test and check the output has NAs only if ifany is selected on the UI.

I haven't added specific tests for this. Probably a shiny::testServer with a snapshot could work to check that the table returns these rows.

App to test

data <- within(teal_data(), {
  ADSL <- tmc_ex_adsl
  ADSL$EOSDY[1] <- NA_integer_
  ind <- 1:100
  ADSL$SEX[ind] <- NA
  # Get code::
  set.seed(1000)
  ind <- sample(seq_len(nrow(ADSL)), 100)
  ADSL$RACE[ind] <- NA
})
join_keys(data) <- default_cdisc_join_keys[names(data)]

init(
  data = data,
  modules = modules(
    tm_t_summary_by(
      label = "Summary by Row Groups Table",
      dataname = "ADSL",
      arm_var = choices_selected(c("ARM", "ARMCD"), "ARM"),
      add_total = TRUE,
      summarize_vars = choices_selected(
        choices = variable_choices("ADSL", c('EOSDY')),
        selected = c('EOSDY')
      ),
      by_vars = choices_selected(
        c("SEX", "RACE", "BMRKR2", "EOSDY", "DCSREAS", "AGE"),
        "SEX"
      ),
      useNA = "ifany"
    ),
    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"),
        "SEX"
      ),
      useNA = "ifany"
    )
  )
) |> runApp()

[github-actions]

Unit Tests Summary

  1 files   71 suites   11s ⏱️
734 tests 155 ✅ 579 💤 0 ❌
874 runs  176 ✅ 698 💤 0 ❌

Results for commit f4bb51f.

♻️ 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$
examples	💚	$7.10$	$-5.46$	$0$	$+115$	$0$	$0$
tm_t_glm_counts	💚	$24.57$	$-24.27$	$-3$	$+2$	$0$	$0$

Additional test case details

Test Suite	$Status$	Time on main	$±Time$	Test Case
tm_t_glm_counts	💚	$12.18$	$-12.00$	e2e_tm_t_glm_counts_Module_initializes_in_teal_without_errors_and_produces_table_output.
tm_t_glm_counts	💚	$12.39$	$-12.28$	e2e_tm_t_glm_counts_Selecting_arm_var_changes_the_table_and_does_not_throw_validation_errors.

Results for commit 21ed094

♻️ This comment has been updated with latest results.

[averissimo]

2 questions:

• The tm_t_summary seems to never display the NAs (see first screenshot with your app)
• Should there be a message when ommiting the NAs in tm_t_summary_by?
  • It looks awkward to see that (N = 69) on the top and then only see observations n 16+ n 14

[llrs-roche]

• Apologies, I have missed something, working on it now.
• This is what the user requested, so they should be aware of it. Depending on how [Bug]: Keeping NAs with analyze_vars(na_rm = TRUE) raise an error tern#1440 is resolved maybe the N at the header changes. But instead of a message, perhaps it could be added at the footnote of the table. Less intrusive and will remain on reports.

[llrs-roche]

I missed an na.rm argument on one analyze_vars(). This should be fixed now.

I also noted that the footnote already says that it counts ID with non-NAs, so I did some modifications. I'm not sure if it is clearer. Let me know if I changed too much the meaning (Same footnote for both tables)

[averissimo]

It looks fine as it is! It corrects the behavior and is more consistent across modules.

• 👍 You have the ok to merge **after** updating the NEWS file.
• I have 1 more comment about consistent naming, but that is not a blocker