Conversation
stats::quantile()'s default type 7 made all simulation-based CI bounds systematically too narrow at small nsim: at the default nsim = 100 the expected central mass of the draws between the bounds is ~93% instead of the nominal 95%. Type-6 quantiles (plotting positions k/(B+1)) remove this systematic inward bias. Applied at all six sim-CI sites: get_sim_ci(), get_sim_ci_cumu(), get_sim_ci_surv(), get_cif.default(), add_trans_ci(), and compute_cumu_diff(). Documents the achieved level and nsim sensitivity in the ci_type/nsim docs, adds a seed-reproduction regression test for the hazard/cumulative-hazard/survival paths, and updates four hardcoded snapshot test values (intervals widen slightly at both ends). Closes #288 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Updates simulation-based confidence interval construction across pammtools to use stats::quantile(..., type = 6) for posterior-draw quantiles, addressing systematic under-coverage at small nsim (fixes #288).
Changes:
- Switch all simulation-based CI bounds to type-6 empirical quantiles in hazard / cumulative-hazard / survival, CIF, transition probabilities, and cumulative-coefficient differences.
- Expand user-facing documentation to describe the type-6 construction and the impact of small
nsimon Monte Carlo noise/achievable nominal level. - Add a regression test that reproduces posterior draws via seeding and verifies bounds match type-6 quantiles; update existing snapshot expectations accordingly.
Reviewed changes
Copilot reviewed 4 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/testthat/test-add-functions.R | Adds a seed-reproduction regression test for type-6 sim-CI bounds; updates expected sim-CI snapshot values. |
| R/cumulative-coefficient.R | Uses type = 6 quantiles for simulated CIs in compute_cumu_diff(). |
| R/add-functions.R | Switches all sim-CI quantile calls to type = 6 (hazard/cumu/surv/CIF/transitions) and documents the behavior. |
| NEWS.md | Notes the behavioral change (slightly wider sim-CIs) and links it to #288. |
| man/get_hazard.Rd | Documents type-6 sim-CI quantiles and nsim guidance/limits. |
| man/get_cumu_hazard.Rd | Documents type-6 sim-CI quantiles and nsim guidance/limits. |
| man/compute_cumu_diff.Rd | Updates CI description to specify type-6 empirical quantiles. |
| man/add_trans_prob.Rd | Documents type-6 quantiles for simulated transition-probability intervals. |
| man/add_hazard.Rd | Documents type-6 sim-CI quantiles and nsim guidance/limits. |
| man/add_cif.Rd | Documents type-6 quantiles for CIF simulated intervals. |
Files not reviewed (6)
- man/add_cif.Rd: Language not supported
- man/add_hazard.Rd: Language not supported
- man/add_trans_prob.Rd: Language not supported
- man/compute_cumu_diff.Rd: Language not supported
- man/get_cumu_hazard.Rd: Language not supported
- man/get_hazard.Rd: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fabian-s
added a commit
that referenced
this pull request
Jun 10, 2026
Reruns scenarios 1, 30, 33 (worst sim100 coverage in the full run) x 100 reps with the patched package from PR #289, exactly paired with reps 1-100 of the stored type-7 results via the deterministic per-(scenario, rep) RNG streams. Pairing verified (delta/default bounds agree to <= 1e-4); sim coverage moves 0.907 -> 0.928 (nsim = 100, theory ~+1.9pp) and 0.919 -> 0.925 (nsim = 500), with near-one-directional coverage flips. See results-type6/README.md for the protocol. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
johannespiller
left a comment
Collaborator
There was a problem hiding this comment.
Review ok. Ready to merge.
Collaborator
Author
|
see #292 |
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.
Closes #288.
Problem
All simulation-based CIs computed interval bounds as empirical
alpha/2/1 - alpha/2quantiles ofnsimposterior coefficient draws viastats::quantile()with its defaulttype = 7. For B draws,E[F(X_(k))] = k/(B+1), so type-7 bounds atB = 100enclose an expected central mass of only ~93.1% instead of the nominal 95% — the intervals are systematically too narrow even if the posterior approximation were perfect (see #288 for the full derivation and the Monte Carlo evidence from the #285 study).Changes
quantile(..., type = 6)(plotting positionsk/(B+1), which remove the systematic inward bias) at all six sim-CI sites:get_sim_ci(),get_sim_ci_cumu(),get_sim_ci_surv()(i.e.add_hazard()/add_cumu_hazard()/add_surv_prob()withci_type = "sim"),get_cif.default()(add_cif()),add_trans_ci()(add_trans_prob()— a site not listed in the issue), andcompute_cumu_diff()(get_cumu_coef()).ci_type/nsimdocs now state the type-6 construction, that the defaultnsim = 100gives noisy bounds (recommend 500+), and thatnsim < 2/alpha - 1(39 for alpha = 0.05) cannot achieve the nominal level at all. NEWS entry notes that all sim-CI bounds widen slightly relative to <= 0.7.5.nsim = 100, exercising the interpolation branch); four hardcoded snapshot values updated (all widen, as the math predicts).Default
nsimvalues are deliberately unchanged (100,add_cif()already at 500): the type fix removes a bias, raisingnsimreduces Monte Carlo noise — an orthogonal, runtime-relevant decision better made separately (issue #288, point 2).Validation
Beyond the unit tests, the fix was validated empirically by rerunning part of the #285 Monte Carlo study (
attic/simulations/ci-surv-probon branchclaude/confident-cori-dng5at; nominal-95% CIs for S(t|x), not included in this PR — verification artifacts and protocol inresults-type6/, commit 7e6a41c): scenarios 1, 30, 33 — the three with the worst sim100 coverage in the full 500-rep run — x 100 reps with the patched package. Since the study's RNG streams are deterministic per (scenario, rep), the rerun is exactly paired with reps 1-100 of the stored type-7 results: point estimates and the (unaffected) delta/default bounds agree to <= 1e-4, so only the quantile type differs.Gains match the order-statistics prediction within Monte Carlo error, and coverage flips are essentially one-directional (type-6 intervals contain the type-7 intervals for the same draws). Residual under-coverage in these worst-case cells (heavy censoring, late times) is the late-hazard fit bias documented in the #285 study — it affects delta and sim CIs alike and is out of scope here.
R CMD check: 0 errors, 0 warnings, 4 pre-existing notes (untouched files). Full test suite passes.🤖 Generated with Claude Code