Skip to content

Add additional lr_stat_df statistic#1474

Open
munoztd0 wants to merge 6 commits into
insightsengineering:mainfrom
munoztd0:main
Open

Add additional lr_stat_df statistic#1474
munoztd0 wants to merge 6 commits into
insightsengineering:mainfrom
munoztd0:main

Conversation

@munoztd0
Copy link
Copy Markdown

@munoztd0 munoztd0 commented May 6, 2026

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
Copy link
Copy Markdown
Author

munoztd0 commented May 6, 2026

@danielinteractive see johnsonandjohnson/junco#158

@Melkiades
Copy link
Copy Markdown
Contributor

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 (orginaloriginal) 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
Copy link
Copy Markdown
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature Request]: tern: Inventory of possible forked tern functions - Survival analysis: Cox PH hazard ratio with CI

3 participants