Error on mis-grouped newdata in cumulative add_* functions#295
Merged
Conversation
The cumulative post-processing functions (add_cumu_hazard, add_surv_prob, add_cif, add_trans_prob; both the default and the pamm_ic methods) now error when newdata is not grouped so that the time variable is unique within each group -- typically a forgotten group_by() -- instead of silently accumulating cumulative quantities across distinct covariate profiles / causes / transitions and returning wrong curves. New internal helper stop_if_undergrouped_for_cumulation() generalises the guard that already existed for the interval-censoring pooling path (and the stopifnot() in the RMST example). All four functions accept check_grouping = FALSE to opt out. The guard detects mis-grouping via repeated time values within a group: make_newdata() grids are always caught, while hand-built grids stacking profiles with disjoint time grids are indistinguishable from a single time-varying-covariate profile and pass undetected (documented). The interval-censoring pooling path is upgraded from a warning to a hard error for consistency. Regression tests in tests/testthat/test-grouping-guard.R; the two test-interval-censored.R assertions that encoded the old warning contract are updated. Full suite: 851 assertions, 0 failures. Addresses the JSS review "silently wrong results" issue (adibender/pammtools_preprint#25). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR makes cumulative post-processing helpers in pammtools fail loudly when newdata is under-grouped in a way that would otherwise yield silently incorrect cumulative estimates (cumulative hazard, survival, CIF, and transition probabilities). It introduces a shared internal guard to detect the common “forgotten group_by()” case (repeated time values within a group), adds an opt-out flag for advanced use, and aligns interval-censoring pooled-method behavior with the new error contract.
Changes:
- Add internal
stop_if_undergrouped_for_cumulation()and wire it intoadd_cumu_hazard(),add_surv_prob(),add_cif(), andadd_trans_prob()(incl.pamm_icmethods), with a newcheck_grouping = TRUEargument and internal opt-outs to avoid redundant checks. - Upgrade the interval-censoring pooled path’s “undergrouped newdata” handling from warning to error for consistency.
- Add/adjust tests and docs (roxygen/Rd + NEWS) and bump version to 0.8.1.
Reviewed changes
Copilot reviewed 7 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/testthat/test-interval-censored.R | Updates expectations to the new “error on under-grouping” contract for pooled IC adders. |
| tests/testthat/test-grouping-guard.R | Adds regression tests covering erroring behavior, correct grouping, and check_grouping = FALSE opt-out. |
| R/pool-ic.R | Replaces the pooled-warning guard with the shared hard-error guard; threads check_grouping through pooled methods and skips redundant per-fit checks. |
| R/cumulative-coefficient.R | Opts out of the grouping check for internal add_cumu_hazard() calls to avoid redundant validation. |
| R/add-functions.R | Implements stop_if_undergrouped_for_cumulation() and adds check_grouping plumbing + updated roxygen across cumulative adders. |
| NEWS.md | Documents the behavior change and the new opt-out flag. |
| man/stop_if_undergrouped_for_cumulation.Rd | Adds generated internal documentation for the new helper. |
| man/add_trans_prob.Rd | Documents check_grouping and the new error behavior for transition probabilities. |
| man/add_surv_prob.Rd | Documents check_grouping and the new error behavior for survival probabilities. |
| man/add_hazard.Rd | Documents check_grouping for add_cumu_hazard() and the updated grouping requirement behavior. |
| man/add_cif.Rd | Documents check_grouping and clarifies interaction with the existing “group by cause” check. |
| DESCRIPTION | Bumps package version/date to 0.8.1 / 2026-07-08. |
Files not reviewed (5)
- man/add_cif.Rd: Generated file
- man/add_hazard.Rd: Generated file
- man/add_surv_prob.Rd: Generated file
- man/add_trans_prob.Rd: Generated file
- man/stop_if_undergrouped_for_cumulation.Rd: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
The cumulative post-processing functions —
add_cumu_hazard(),add_surv_prob(),add_cif(),add_trans_prob()(both the default and thepamm_icmethods) — previously returned silently incorrect results whennewdatawas not grouped so that the time variable is unique per group (a forgottengroup_by()): the cumulative sum ran across distinct covariate profiles / causes / transitions. The package documented this footgun rather than preventing it.This PR makes the failure loud: a new internal helper
stop_if_undergrouped_for_cumulation()errors when a group contains repeated time values, generalising the guard that already existed for the interval-censoring pooling path (and thestopifnot()in the RMST example). The interval-censoring pooling path itself is upgraded from a warning to a hard error for consistency.Details
check_grouping = TRUEargument on all four functions; passcheck_grouping = FALSEto opt out (e.g. advanced workflows that intentionally accumulate over rows with varying covariates). Internal call sites (add_trans_prob→add_cumu_hazard,cumulative-coefficient.R, thepamm_icper-fit calls) opt out to avoid a redundant re-check.make_newdata()are always caught; hand-built grids that stack profiles with disjoint time grids are indistinguishable from a single profile with time-varying covariates and pass undetected.NEWS.mdupdated; version bumped to 0.8.1 (0.8.0 is on CRAN).Tests
New
tests/testthat/test-grouping-guard.R(ungrouped multi-profile → error for all four functions; single-profile no false positive;check_grouping = FALSEopt-out incl. thepamm_icmethods; grouped → no error). The twotest-interval-censored.Rassertions that encoded the old warning contract are updated. Full suite: 851 assertions, 0 failures / 0 warnings / 0 skips.Context
Addresses the "silently wrong results" issue raised in the JSS peer review of the
pammtoolssoftware article (adibender/pammtools_preprint#25). Agit apply-able copy of this change is also carried in that repo underpammtools-patches/.🤖 Generated with Claude Code