Add option to tm_t_summary to include arm_var labels in table header

[edelarua]

Pull Request

Fixes #1204

Also deprecates show_labels argument to template_summary that's not used in the module.

[github-actions]

Unit Tests Summary

    1 files     70 suites   1h 6m 21s ⏱️
  722 tests   612 ✅ 110 💤 0 ❌
1 988 runs  1 765 ✅ 223 💤 0 ❌

Results for commit d85f51a.

♻️ This comment has been updated with latest results.

[github-actions]

Unit Test Performance Difference

Test suite performance difference

Test Suite	$Status$	Time on main	$±Time$	$±Tests$	$±Skipped$	$±Failures$	$±Errors$
shinytest2-tm_a_mmrm	💔	$699.12$	$+3.21$	$0$	$0$	$0$	$0$
shinytest2-tm_g_barchart_simple	💔	$220.50$	$+1.76$	$0$	$0$	$0$	$0$
shinytest2-tm_g_ci	💔	$97.44$	$+1.31$	$0$	$0$	$0$	$0$
shinytest2-tm_g_forest_rsp	💔	$166.51$	$+3.50$	$0$	$0$	$0$	$0$
shinytest2-tm_g_ipp	💔	$98.54$	$+1.81$	$0$	$0$	$0$	$0$
shinytest2-tm_g_km	💔	$253.20$	$+1.85$	$0$	$0$	$0$	$0$
shinytest2-tm_g_lineplot	💔	$84.13$	$+1.52$	$0$	$0$	$0$	$0$
shinytest2-tm_g_pp_therapy	💚	$176.22$	$-2.30$	$0$	$0$	$0$	$0$
shinytest2-tm_t_abnormality_by_worst_grade	💔	$63.23$	$+1.68$	$0$	$0$	$0$	$0$
shinytest2-tm_t_ancova	💔	$87.08$	$+2.63$	$0$	$0$	$0$	$0$
shinytest2-tm_t_binary_outcome	💔	$70.72$	$+1.94$	$0$	$0$	$0$	$0$
shinytest2-tm_t_coxreg	💔	$68.32$	$+2.01$	$0$	$0$	$0$	$0$
shinytest2-tm_t_events	💔	$56.08$	$+1.58$	$0$	$0$	$0$	$0$
shinytest2-tm_t_events_by_grade	💔	$85.73$	$+1.87$	$0$	$0$	$0$	$0$
shinytest2-tm_t_events_summary	💔	$58.66$	$+1.28$	$0$	$0$	$0$	$0$
shinytest2-tm_t_exposure	💔	$71.42$	$+1.00$	$0$	$0$	$0$	$0$
shinytest2-tm_t_pp_laboratory	💔	$120.22$	$+2.98$	$0$	$0$	$0$	$0$
shinytest2-tm_t_pp_medical_history	💔	$62.18$	$+1.62$	$0$	$0$	$0$	$0$
shinytest2-tm_t_shift_by_arm	💔	$54.21$	$+2.07$	$0$	$0$	$0$	$0$
shinytest2-tm_t_shift_by_arm_by_worst	💔	$83.92$	$+2.18$	$0$	$0$	$0$	$0$
shinytest2-tm_t_shift_by_grade	💔	$73.75$	$+3.70$	$0$	$0$	$0$	$0$
shinytest2-tm_t_smq	💔	$54.13$	$+2.07$	$0$	$0$	$0$	$0$
shinytest2-tm_t_summary_by	💔	$74.10$	$+1.52$	$0$	$0$	$0$	$0$
shinytest2-tm_t_tte	💔	$60.35$	$+1.93$	$0$	$0$	$0$	$0$

Additional test case details

Test Suite	$Status$	Time on main	$±Time$	Test Case
tm_t_summary	👶		$+0.02$	template_summary_generates_correct_expressions_when_arm_variable_labels_are_added

Results for commit 8c65c23

♻️ This comment has been updated with latest results.

[ayogasekaram]

Hey @edelarua! The changes work well but I had a quick note - I tried running an example and there seems to be some indenting applied but the variables aren't nested:

[shajoezhu]

I think we need to add a check step, such that if nest table make sense, check if variables X and Y are not 1 to 1 mapping in the data,

[edelarua]

Hey @edelarua! The changes work well but I had a quick note - I tried running an example and there seems to be some indenting applied but the variables aren't nested

Hi @ayogasekaram,

Technically the column variables are nested which is why I added the indentation, but I'm not sure if it makes sense to include since the nesting is done in the column direction and not the row direction as the indent somewhat implies. Do you think it would make more sense to remove all indentation?

[ayogasekaram]

Hey @edelarua! The changes work well but I had a quick note - I tried running an example and there seems to be some indenting applied but the variables aren't nested

Hi @ayogasekaram,

Technically the column variables are nested which is why I added the indentation, but I'm not sure if it makes sense to include since the nesting is done in the column direction and not the row direction as the indent somewhat implies. Do you think it would make more sense to remove all indentation?

I think the indenting might be confusing because that is how row space nesting is shown in the table. I think removing all indenting for this header would be clearer but I'm open to both options! Whatever you think is clearer :)

[Melkiades]

Hey @edelarua! The changes work well but I had a quick note - I tried running an example and there seems to be some indenting applied but the variables aren't nested:

This could be as_html again. I think there may be some manual indentation or some assumption on the fact that there is another cell-value there

[edelarua]

This could be as_html again. I think there may be some manual indentation or some assumption on the fact that there is another cell-value there

@Melkiades I added the indentation in myself so it's easy to remove. Not related to as_html thankfully :)

[edelarua]

I think we need to add a check step, such that if nest table make sense, check if variables X and Y are not 1 to 1 mapping in the data,

@shajoezhu do we want to impose this with a check? I would assume users would not intentionally choose to put 1:1 variables in the col splits, but there's no detriment (it doesn't affect the functionality) if they do.

[ayogasekaram]

lgtm! thanks @edelarua

[shajoezhu]

I think we need to add a check step, such that if nest table make sense, check if variables X and Y are not 1 to 1 mapping in the data,

@shajoezhu do we want to impose this with a check? I would assume users would not intentionally choose to put 1:1 variables in the col splits, but there's no detriment (it doesn't affect the functionality) if they do.

hi @edelarua , I think what we have now is good. I think a check would be good to remind users about the levels. i think assertion is too strong, we don't need that, reduce the header level is too aggressive, people may not like that. another possibility that users may raise is that is to add zero entries.

let's leave this as it is for now. and wait for some feedback. thanks!