You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Purpose:
Fixes johnsonandjohnson/junco#158