Add additional lr_stat_df statistic

[munoztd0]

Pull Request

Purpose:

• Adds lr_stat_df (log-rank statistic with degrees of freedom) in addition to the usual stats.
• Adds alternative = c("two.sided", "less", "greater") logic

Fixes johnsonandjohnson/junco#158

[munoztd0]

@danielinteractive see johnsonandjohnson/junco#158

[Melkiades]

Hey, thanks for working on this! A few thoughts:

lr_stat_df not registered in defaults — The new statistic is returned by s_coxph_pairwise() but isn't added to tern_default_stats or tern_default_formats in utils_default_stats_formats_labels.R (line 577). Without that, it won't show up in tables unless the user explicitly requests it via .stats, and even then it won't have a format. See how hr_ci_3d is handled for reference — it's in the return list and has a format entry even though it's not in the defaults.

alternative placement — In tern, the existing precedent for alternative is in s_test_proportion_diff() (in prop_diff_test.R), where it's passed directly to the underlying stats functions (prop.test, fisher.test, mantelhaen.test) that handle one-sided logic natively. Here, the p-value halving is done manually (pval / 2 or 1 - pval / 2), which is a different pattern. A couple of concerns:

• The halving approach works for Wald and likelihood ratio tests, but for the log-rank test from survdiff() the chi-squared statistic is inherently non-directional. A proper one-sided log-rank test would use the signed Z-statistic (sign(HR - 1) * sqrt(chisq)), not just halving the two-sided p-value. The approximation is common in practice but worth documenting if intentional.
• The direction check uses the Cox model HR (sum_cox$conf.int[1, 1] < 1) regardless of pval_method, which mixes the Cox estimate with the log-rank statistic.
• It might also be worth considering whether alternative should live in control_coxph() to keep the API consistent — that's where pval_method, ties, and conf_level already are.

Runtime assertion — The checkmate::assert_true(all.equal(log_rank_pvalue, original_survdiff$pvalue)) line will crash the function if there's a floating-point rounding difference. This could happen with edge-case data. If the goal is to validate the d.f. calculation, a test assertion (expect_equal with tolerance) would be safer than a runtime check.

d.f. calculation — The manual recomputation of degrees of freedom from original_survdiff$exp duplicates internal logic from survival::survdiff. For a two-group pairwise comparison (which is what s_coxph_pairwise always does — it compares .ref_group vs df), the d.f. is always 1. A simpler approach would be c(original_survdiff$chisq, length(original_survdiff$n) - 1).

Minor things:

• The typo fix (orginal → original) is a nice catch 👍
• The NEWS entry combines two changes in one bullet — tern convention is one bullet per logical change.
• The extra expect_warning wrappers in the NA test (2 → 4) could use a comment explaining what the additional warnings are.
• The snapshot diff shows trailing whitespace/alignment changes unrelated to the feature.

[danielinteractive]

Thanks @Melkiades for your review, I have helped with some stats related changes here - please have a look:

• simplified d.f. calculation
• removed runtime assertion
• used signed LR statistic for one-sided p-value

If that is fine then @munoztd0 can take care of other changes afterwards.

[Melkiades]

Thanks @Melkiades for your review, I have helped with some stats related changes here - please have a look:

• simplified d.f. calculation
• removed runtime assertion
• used signed LR statistic for one-sided p-value

If that is fine then @munoztd0 can take care of other changes afterwards.

Thanks Daniel and @munoztd0!! Great work. I have only a couple of minor issues and one question about the pvalue ^^

[danielinteractive]

Thanks @Melkiades !
@munoztd0 can you take care of the remaining comments?

[munoztd0]

Thanks @Melkiades ! @munoztd0 can you take care of the remaining comments?

Yes I will do ! thanks for the rest

[munoztd0]

Hey, thanks for working on this! A few thoughts:

lr_stat_df not registered in defaults — The new statistic is returned by s_coxph_pairwise() but isn't added to tern_default_stats or tern_default_formats in utils_default_stats_formats_labels.R (line 577). Without that, it won't show up in tables unless the user explicitly requests it via .stats, and even then it won't have a format. See how hr_ci_3d is handled for reference — it's in the return list and has a format entry even though it's not in the defaults.

alternative placement — In tern, the existing precedent for alternative is in s_test_proportion_diff() (in prop_diff_test.R), where it's passed directly to the underlying stats functions (prop.test, fisher.test, mantelhaen.test) that handle one-sided logic natively. Here, the p-value halving is done manually (pval / 2 or 1 - pval / 2), which is a different pattern. A couple of concerns:

• The halving approach works for Wald and likelihood ratio tests, but for the log-rank test from survdiff() the chi-squared statistic is inherently non-directional. A proper one-sided log-rank test would use the signed Z-statistic (sign(HR - 1) * sqrt(chisq)), not just halving the two-sided p-value. The approximation is common in practice but worth documenting if intentional.
• The direction check uses the Cox model HR (sum_cox$conf.int[1, 1] < 1) regardless of pval_method, which mixes the Cox estimate with the log-rank statistic.
• It might also be worth considering whether alternative should live in control_coxph() to keep the API consistent — that's where pval_method, ties, and conf_level already are.

Runtime assertion — The checkmate::assert_true(all.equal(log_rank_pvalue, original_survdiff$pvalue)) line will crash the function if there's a floating-point rounding difference. This could happen with edge-case data. If the goal is to validate the d.f. calculation, a test assertion (expect_equal with tolerance) would be safer than a runtime check.

d.f. calculation — The manual recomputation of degrees of freedom from original_survdiff$exp duplicates internal logic from survival::survdiff. For a two-group pairwise comparison (which is what s_coxph_pairwise always does — it compares .ref_group vs df), the d.f. is always 1. A simpler approach would be c(original_survdiff$chisq, length(original_survdiff$n) - 1).

Minor things:

• The typo fix (orginal → original) is a nice catch 👍
• The NEWS entry combines two changes in one bullet — tern convention is one bullet per logical change.
• The extra expect_warning wrappers in the NA test (2 → 4) could use a comment explaining what the additional warnings are.
• The snapshot diff shows trailing whitespace/alignment changes unrelated to the feature.

@Melkiades thanks for your review and comments

I have tackled all remaining issues I believe

except :

• The snapshot diff shows trailing whitespace/alignment changes unrelated to the feature.

because I believe that if you are indeed talking about those whitespace there are related to the feature which adds p-value and make width wider

Other than that I have fixed:

• The NEWS entry combines two changes in one bullet — tern convention is one bullet per logical change.
  lr_stat_df not registered in defaults

@danielinteractive has fixed everything else I believe

[Melkiades]

Amazing work! Thank you both for acting fast and precisely! Good to go for me :)

[munoztd0]

Amazing work! Thank you both for acting fast and precisely! Good to go for me :)

Sorry, I tried to rerun the checks because they were all cancelled but re triggered the workflow approval also @Melkiades