Skip to content

fix: label by= strata from the fit, so subset= can drop a group - #310

Merged
ehrlinger merged 3 commits into
mainfrom
fix/strata-kept-by-fit
Sep 30, 2026
Merged

ehrlinger merged 3 commits into
mainfrom
fix/strata-kept-by-fit

Conversation

@ehrlinger

@ehrlinger ehrlinger commented Sep 30, 2026 •

Copy link
Copy Markdown
Owner

Follow-up to #305, raised by Copilot in review on #308.

What was wrong

kaplan() and nelson() pass ... to survfit(). An option there that removes a whole group (subset, start.time) leaves the fit with fewer strata than the by column has values.

The change

  • by is turned into a factor bound to the name grp (.strata_factor()), and both fits use survfit(srv ~ grp, ...). survfit() then names each stratum grp=<level>, and .label_strata() reads the labels back from those names, mapping them to the original values so groups keeps the column's type. The fit is the same as with strata(data[[by]]); I checked surv and the strata counts on veteran.
  • When subset leaves a single group, survfit() returns no $strata, so there is nothing to read. That case takes the group from the rows the fit used: complete cases inside the subset subscript, applied to the row numbers so logical, positive and negative subscripts all work (.fit_rows()). A single stratum left by start.time keeps its name and is read like any other. If the kept rows do not come to one group, it is an error that says to subset data instead.
  • nelson()'s weighted fit uses the same grp.

Verification

  • New test, for both estimators: subset leaving one group (labelled 2 and equal to that arm fitted alone); a negative subset leaving the other; start.time = 200 dropping adeno, and start.time = 600 leaving only squamous; and subset dropping one of four celltype groups (labels and surv match each group fitted alone).
  • devtools::document(): no changes. lintr::lint_package(): 0.
  • NOT_CRAN=true VDIFFR_RUN_TESTS=true devtools::test(): 0 failures, 2317 passed, 6 skipped (existing skips). No snapshot files touched.
  • R CMD check --as-cran with the manual, from a git archive export: 1 NOTE (CRAN incoming feasibility).

A backport to maint/v3 is ready locally and will follow once this merges.

🤖 Generated with Claude Code

ehrlinger and others added 2 commits September 30, 2026 15:27
kaplan() and nelson() counted the by= groups from the data and handed
out labels by position, so a survfit() option passed through ... that
removed a whole group (subset, start.time) stopped with "the 'by'
column has 2 groups but the fit has 1 strata". CRAN 3.5.3 relabelled the
remaining groups instead. Fit on a factor bound to `grp`, so survfit()
names each stratum "grp=<level>", and read the labels back from those
names. A fit left with one stratum is unnamed; its group is found from
the rows the fit used.

Raised in review on #308.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings September 30, 2026 21:11
@codecov

codecov Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.71429% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.14%. Comparing base (4c0b1ec) to head (3810943).

Files with missing lines Patch % Lines
R/utils.R 79.16% 5 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #310      +/-   ##
==========================================
- Coverage   91.15%   91.14%   -0.02%     
==========================================
  Files          60       60              
  Lines        5949     5964      +15     
==========================================
+ Hits         5423     5436      +13     
- Misses        526      528       +2     
Files with missing lines Coverage Δ
R/kaplan.R 100.00% <100.00%> (ø)
R/nelson.R 96.96% <100.00%> (+0.04%) ⬆️
R/utils.R 96.09% <79.16%> (-1.33%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Negative row-index subsets are mishandled, and the documented start.time behavior lacks regression coverage.

Review effort: Balanced
Findings: 1 Medium severity · 1 Low severity

Open (2)
What changed in this PR

Updates stratified Kaplan–Meier and Nelson–Aalen labeling when survfit() options remove groups.

Changes:

  • Derives labels from fitted strata.
  • Handles single-stratum fits using retained rows.
  • Adds subset-based regression coverage.
File Description
R/​utils.R Adds stratification and fit-row helpers.
R/​kaplan.R Uses fit-derived group labels.
R/​nelson.R Applies matching labels to weighted and unweighted fits.
tests/​testthat/​test_kaplan_nelson.R Tests groups removed by subset.
NEWS.md Documents corrected labeling.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread R/utils.R Outdated
Comment on lines +71 to +72
} else if (!is.null(subset)) {
kept <- kept & seq_along(kept) %in% subset

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed: with subset = -arm_2 the fit kept only trt == 1 and label recovery rejected it. Fixed in 3810943: the subscript is applied to the row numbers (seq_along(kept)[subset]), so logical, positive and negative subscripts mean what they mean to survfit(). The test adds the negative case for both estimators.

Comment on lines +360 to +367
vet <- survival::veteran
for (est in list(kaplan, nelson)) {
one <- est(interval = "time", censor = "status", data = vet, by = "trt",
subset = vet$trt == 2)
expect_equal(unique(one$groups), 2)
expect_equal(one$surv,
est(interval = "time", censor = "status",
data = vet[vet$trt == 2, ])$surv)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the test did not cover start.time, and checking it showed my PR description was also wrong. A start.time that leaves a single stratum does not lose its name: survfit() still returns grp=squamous, so it is labelled from the name and does not error. Only a subset down to one group returns no $strata. Added in 3810943: start.time = 200 drops adeno (last time 186) and labels the other three; start.time = 600 leaves only squamous, labelled correctly. Description corrected.

Review on #310. .fit_rows() tested row numbers with %in% subset, so an
exclusion such as subset = -(1:2) selected no rows and a fit left with
one group failed label recovery. Also pins start.time: dropping an early
group, and leaving one stratum, which survfit() still names.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ehrlinger
ehrlinger merged commit b509778 into main Sep 30, 2026
10 checks passed
@ehrlinger
ehrlinger deleted the fix/strata-kept-by-fit branch September 30, 2026 23:46
ehrlinger added a commit that referenced this pull request Oct 1, 2026
…int-v3

Backport #310 to maint/v3: label by= strata from the fit
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.

2 participants