Skip to content

Fix get_cox_pairwise_df returning NA hazard ratios when arm has >2 levels#194

Open
Copilot wants to merge 6 commits intomainfrom
copilot/fix-get-cox-pairwise-hazard-ratio
Open

Fix get_cox_pairwise_df returning NA hazard ratios when arm has >2 levels#194
Copilot wants to merge 6 commits intomainfrom
copilot/fix-get-cox-pairwise-hazard-ratio

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 24, 2026

get_cox_pairwise_df always reads row 1 from coxph_ans$conf.int, but when the arm factor has >2 levels, subsetting the data doesn't drop unused factor levels — so conf.int still contains a row per non-reference level (with NA for absent ones). Only the first comparison returns valid results; all subsequent ones return NA.

Fix

Replace the hard-coded row index 1 with paste0(arm, current_arm), matching R's factor dummy variable naming convention:

# Before — always reads first row regardless of which arm is current
hr = sprintf("%.2f", coxph_ans$conf.int[1, 1])

# After — reads the row for the current comparison arm
conf_int_row <- paste0(arm, current_arm)
hr = sprintf("%.2f", coxph_ans$conf.int[conf_int_row, 1])

Tests

Added test-get_cox_pairwise_df.R covering:

  • 3-arm case returns non-NA HR and CI for all comparisons (the regression case)
  • 2-arm case continues to work
  • Default ref_group falls back to first factor level
  • Input validation errors
Original prompt

This section details on the original issue you should resolve

<issue_title>get_cox_pairwise_df returns hazard ratio of NA if by variable has more than two levels</issue_title>
<issue_description>The issue resides in the get_cox_pairwise_df function. The idea of this function is that if your by variable has n levels, it computes n-1 cox proportional hazards model, each time comparing one level to the reference level. However, the function is only working correctly if you have two levels. If you have more than two levels, all comparisons after the first one will return NA for the hazard ratio and confidence interval.

The reason for this is that the function initiates a for loop and in each iteration, subsets the data to just two levels (reference and one other) and then computes the cox model. However, because the by variable is a factor it still remembers that the other levels exists so the cox proportional hazard model will still include all levels in the summary with a hazard ratio of NA. This is fine as long as we are careful to return the correct row in the output but the function is always returning the first row of the summary when in fact, it should return the ith row for the ith iteration of the for loop.

In simple terms, we need to update rows 104, 107 and 109 of the function to reference the ith iteration rather than hard-coding this to 1, highlighted below:

Image</issue_description>

<agent_instructions>follow instruction from @reesnj to propose the PR</agent_instructions>

Comments on the Issue (you are @copilot in this section)


📱 Kick off Copilot coding agent tasks wherever you are with GitHub Mobile, available on iOS and Android.

Copilot AI changed the title [WIP] Fix get_cox_pairwise_df to handle multiple levels in by variable Fix get_cox_pairwise_df returning NA hazard ratios when arm has >2 levels Mar 24, 2026
Copilot AI requested a review from shajoezhu March 24, 2026 12:27
@shajoezhu shajoezhu marked this pull request as ready for review March 24, 2026 14:32
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 24, 2026

Unit Tests Summary

  1 files  144 suites   2m 24s ⏱️
144 tests 144 ✅ 0 💤 0 ❌
423 runs  423 ✅ 0 💤 0 ❌

Results for commit 4226e72.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 24, 2026

badge

Code Coverage Summary

Filename                                 Stmts    Miss  Cover    Missing
-------------------------------------  -------  ------  -------  ------------------------------------------------------------------------------------------------
R/add_blank_rows.R                          63       0  100.00%
R/add_difference_row.R                     101       0  100.00%
R/add_forest_utils.R                        97      10  89.69%   76-79, 94-100
R/add_forest.R                             139       0  100.00%
R/add_hierarchical_count_row.R              33       0  100.00%
R/annotate_gg_km.R                         135       0  100.00%
R/annotate_gg_pkc.R                         85       0  100.00%
R/ard_tabulate_abnormal_by_baseline.R       65       0  100.00%
R/crane-package.R                            2       2  0.00%    26-27
R/deprecated.R                               6       6  0.00%    15-21
R/get_cox_pairwise_df.R                     51       4  92.16%   92-95
R/gg_km_utils.R                             35      14  60.00%   18-35
R/gg_km.R                                  143      37  74.13%   54-57, 74, 101, 175-180, 183-186, 196-198, 203-204, 238-240, 247-250, 254, 265-269, 282, 284-286
R/gg_lineplot.R                             99       2  97.98%   145, 148
R/gg_mmrm_lineplot.R                       102       1  99.02%   106
R/gg_pkc_lineplot.R                         80       0  100.00%
R/gg_utils.R                               168       0  100.00%
R/label_roche.R                             72       0  100.00%
R/modify_header_rm_md.R                     18       2  88.89%   35-36
R/modify_zero_recode.R                      13       0  100.00%
R/reverse_difference_ci.R                   33       0  100.00%
R/tbl_baseline_chg.R                       186       0  100.00%
R/tbl_hierarchical_rate_and_count.R        148       0  100.00%
R/tbl_hierarchical_rate_by_grade.R         271       3  98.89%   162-164
R/tbl_listing.R                             35       0  100.00%
R/tbl_mmrm.R                               254       1  99.61%   392
R/tbl_null_report.R                          9       0  100.00%
R/tbl_rmpt.R                               157      12  92.36%   297-302, 314-319
R/tbl_roche_subgroups.R                    128       0  100.00%
R/tbl_roche_summary.R                       64       0  100.00%
R/tbl_shift.R                              116       0  100.00%
R/tbl_survfit_quantiles.R                  132       1  99.24%   295
R/tbl_survfit_times.R                       92       0  100.00%
R/theme_gtsummary_roche.R                   78       0  100.00%
R/utils.R                                   36       0  100.00%
TOTAL                                     3246      95  97.07%

Diff against main

Filename                   Stmts    Miss  Cover
-----------------------  -------  ------  -------
R/get_cox_pairwise_df.R       +1      -9  +18.16%
TOTAL                         +1      -9  +0.28%

Results for commit: 4226e72

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 24, 2026

Unit Test Performance Difference

Test Suite $Status$ Time on main $±Time$ $±Tests$ $±Skipped$ $±Failures$ $±Errors$
get_cox_pairwise_df 👶 $+0.00$ $+1$ $0$ $0$ $0$
modify_header_rm_md 💔 $0.27$ $+2.53$ $0$ $0$ $0$ $0$
Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
add_blank_rows 💔 $6.75$ $+1.09$ add_blank_rows_works
add_forest 💚 $2.02$ $-1.71$ add_forest_table_engine_flextable_works
get_cox_pairwise_df 👶 $+0.00$ get_cox_pairwise_df_errors_when_arm_column_is_not_a_factor
get_cox_pairwise_df 👶 $+0.00$ get_cox_pairwise_df_errors_with_non_formula_model_formula
get_cox_pairwise_df 👶 $+0.01$ get_cox_pairwise_df_returns_non_NA_hazard_ratios_with_more_than_two_arms
get_cox_pairwise_df 👶 $+0.00$ get_cox_pairwise_df_uses_first_factor_level_as_default_ref_group
get_cox_pairwise_df 👶 $+0.24$ get_cox_pairwise_df_works_with_two_arms
modify_header_rm_md 💔 $0.27$ $+2.53$ strip_md_bold_works_with_gtsummary_table
tbl_hierarchical_rate_and_count 💔 $17.61$ $+3.22$ tbl_hierarchical_rate_and_count_works
tbl_hierarchical_rate_by_grade 💔 $12.78$ $+7.35$ tbl_hierarchical_rate_by_grade_works
tbl_rmpt 💔 $15.76$ $+1.36$ tbl_rmpt_works_with_default_parameters_Example_1_
tbl_roche_subgroups 💔 $3.36$ $+2.09$ tbl_roche_subgroups_time_to_event_NULL_works
tbl_roche_summary 💚 $6.90$ $-4.49$ tbl_roche_summary_works

Results for commit fd018ee

♻️ This comment has been updated with latest results.

@Melkiades
Copy link
Copy Markdown
Contributor

@jszczypinski as you are working on this next, could you take a look and merge or replace this? Thanks :)

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.

get_cox_pairwise_df returns hazard ratio of NA if by variable has more than two levels

4 participants