Fix group-sequential threshold misalignment for hypotheses first tested after analysis 1#2
Open
lhunt13 wants to merge 1 commit into
Conversation
…ed after analysis 1 In update_p_thresholds_cpp, per-look rejection boundaries were written into the threshold matrix starting at analysis column 1, while get_maurer_bretz_z_raw_cpp places observed p-values at the hypothesis's analyses_analysed columns. For any multi-look hypothesis whose first tested analysis is not analysis 1 (e.g. OS tested at analyses 2 & 3), the stringent interim boundary landed on an analysis where the hypothesis is never tested and was silently discarded, while the real interim look was tested against the final-look boundary. This inflated simulated unconditional power (Tables 6a/6b) above the hypothesis's full-alpha local power and over-spent alpha; design boundaries and local power (Tables 1/4/5) were unaffected. Thresholds are now indexed by analyses_analysed, mirroring the p-value placement, and forward-filled across later analyses to match the existing forward-fill of observed p-values (so a hypothesis retested after weight propagation uses its most recent look's boundary). Columns before a hypothesis's first look keep the -1 sentinel, matching NA p-values there. Adds regression tests: exact boundary placement for a staggered (analyses_analysed = 2:3) hypothesis, and the invariant that simulated unconditional power cannot exceed full-alpha local power. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Problem
In the simulation-based operating characteristics,
update_p_thresholds_cppwrites each hypothesis's per-look rejection boundaries into the threshold matrix starting at analysis column 1, whileget_maurer_bretz_z_raw_cppplaces the observed p-values at the hypothesis'sanalyses_analysedcolumns. The two are consistent only when a hypothesis's first tested analysis is analysis 1.For any multi-look (alpha-spending) hypothesis first tested later — e.g. an OS endpoint tested at analyses 2 & 3 — the stringent interim boundary lands on an analysis where the hypothesis is never tested (and is silently discarded), while its real interim look is tested against the final-look boundary. Simulated unconditional power (Tables 6a/6b) is inflated above the hypothesis's full-alpha local power (an upper bound), and the local test over-spends alpha (e.g. cumulative Type-I error 0.033 vs the 0.025 target at full weight).
Design boundaries, nominal p-values, and local power (Tables 1/4/5) are computed correctly; only the simulated operating characteristics and quantities derived from them (expected success analysis/time, subset powers) are affected.
This also fires in the bundled
platform_studyexample. Re-running it patched vs unpatched (same seed), Table 6a changes only for the staggered multi-look hypotheses — power at their interim (analysis 2) drops by up to 10.5 percentage points (H8/H9: 0.657 → 0.552; H10/H11: 0.266 → 0.227; H3–H7: 0.913 → 0.894) — while hypotheses first tested at analysis 1 (H1, H2) and the single-look H12 are bit-for-bit unchanged, exactly matching the bug's scope.Minimal demonstration
Two asOF hypotheses, H1 tested at analyses 1 & 2, H2 (OS) at analyses 2 & 3, all alpha starting on H1:
Before the fix, H2's simulated unconditional power at its interim (57.6%) exceeded its full-alpha local power there (47.2%), which is impossible for a valid alpha-spending boundary. After the fix it is 45.3% / 70.6% — properly bounded by the 47.2% / 72.7% local power.
Fix
update_p_thresholds_cppnow indexes thresholds byanalyses_analysed, mirroring the p-value placement inget_maurer_bretz_z_raw_cpp:analyses_analysedcolumn.p_obs, so a hypothesis retested at a later global analysis (after weight propagation) is tested against its most recent look's boundary rather than an unrelated one.-1sentinel;p_obsisNAthere, so the hypothesis can never be spuriously rejected at analyses preceding its first look.Behavior is unchanged for hypotheses whose looks start at analysis 1, and for single-look hypotheses.
Tests
New
tests/testthat/test-sim-thresholds.R:analyses_analysed = 2:3), the interim/final boundaries land at analysis columns 2/3, column 1 stays at the-1sentinel, and a hypothesis starting at analysis 1 keeps its previous placement including the carried-forward final boundary.Both tests fail on
mainand pass with this change. Fulltestthatsuite passes (0 failures, 106 passing);R CMD checkis clean apart from the two notes already present onmain(gsDesign2:::imports and the foreign.C("gsbound1")call).NEWS.mdupdated.Possibly-separate observation (not addressed here)
After this fix,
platform_studystill reports simulated power above the full-alpha analytic local power for some OS hypotheses (e.g. H1: 0.744 vs 0.723 at its first look; H8: 0.552 vs 0.350). This is unrelated to the threshold alignment: it also affects H1, whose looks start at analysis 1 and whose Table 6a values are identical before and after this patch. It may be a design-vs-simulation information mismatch specific to OS endpoints in that example, or expected behavior of the platform design; happy to investigate and file separately.🤖 Generated with Claude Code