v0.5 seed & RNG-stream contract (#38)#48
Merged
Merged
Conversation
Records the seed/RNG-stream contract for v0.5: L'Ecuyer-CMRG stage sub-streams via parallel::nextRNGStream(), unified save/restore isolation across native/simstudy/legacy paths, RNG-kind pinning within the generation scope, and an always-on fallback stream-switch message. Co-released with the formula evaluator (#39) so v0.5 carries exactly one seeded-output break. Fulfils #22; resolves #33 item 11. Implementation is a separate ADR-gated plan.
Three-task plan implementing the accepted v0.5 seed-contract ADR: L'Ecuyer-CMRG per-stage sub-streams, unified path isolation, always-on fallback message, and the budgeted seeded-output re-baseline.
Replace the seed/seed+1L split with named, provably-independent sub-streams (baseline, postprocess) derived from one public seed via parallel::nextRNGStream(). .with_mock_seed() now pins RNGkind within scope and restores it, so output no longer depends on the caller's ambient RNGkind(). Native, postprocess, and simstudy paths route through it. Re-baselines value-pinning tests for the RNG change.
The RNGkind-independence test switched the ambient generator to Marsaglia-Multicarry, whose built-in "poor statistical properties" caution fired twice (the switch and the on.exit restore). Wrap the switch and its generation in suppressWarnings; the test's semantics (output identical across ambient kinds) are unchanged.
Route the legacy generation loop through .with_mock_seed() so it no longer clobbers the caller's RNG stream, matching the native path. Emit an always-on message when the v0.4 path silently falls back to the legacy generator for an unsupported feature (resolves #33 item 11). Re-baselines affected legacy value-pin tests.
Add cross-path determinism and pinned-reference tests, harmonize the seed @param docs across the three generators, and document the v0.5 reproducibility break in NEWS. Fulfils #22.
Whole-branch review found that set.seed(kind = "L'Ecuyer-CMRG") pinned only the primary generator, so rnorm() and equal-probability sample() still leaked the caller's ambient normal.kind/sample.kind - partially breaking the ADR D3 'independent of ambient RNGkind()' guarantee. Pin all three components (a no-op under R >= 3.6 defaults, so pinned values are unchanged) and add a regression test that varies normal.kind and sample.kind. Scope the NEWS isolation claim to seeded generation calls.
The on.exit restore of the ambient sample.kind = Rounding re-triggered R's caution during the second generation call; extend suppressWarnings to enclose it. Test semantics (output identical across ambient kinds) unchanged.
There was a problem hiding this comment.
Pull request overview
Implements the v0.5 seed & RNG-stream contract across generation paths by centralizing all seeded randomness in a staged .with_mock_seed() wrapper that uses L'Ecuyer-CMRG sub-streams and restores the caller’s RNG state/kind, with accompanying tests and release documentation.
Changes:
- Introduces fixed stage → sub-stream mapping and updates native/simstudy/postprocess + legacy generation paths to use
.with_mock_seed()with stage-specific sub-streams. - Removes the previous
seed/seed + 1Lsplit and adds an always-on legacy-fallback message when v0.4 features are unsupported. - Adds a comprehensive seed-contract test suite plus NEWS and roxygen updates; adds
parallelto Imports.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/testthat/test-seed-contract.R | Adds contract tests for determinism, stage independence, and RNG isolation/kind-independence. |
| R/mock_spec_native.R | Adds .MOCK_STAGES, extends .with_mock_seed() to stage-based L'Ecuyer sub-streams, wires native baseline stage. |
| R/mock_spec_postprocess.R | Routes post-processing through the "postprocess" stage sub-stream; updates seed docs. |
| R/mock_spec_simstudy.R | Routes simstudy baseline generation through the "baseline" stage sub-stream; updates seed docs. |
| R/create_mock_data.R | Drops seed+1L, wraps legacy loop with .with_mock_seed(), adds always-on fallback message. |
| NEWS.md | Documents the v0.4 → v0.5 seeded-output breaking change and RNG isolation guarantee. |
| DESCRIPTION | Adds base parallel to Imports for nextRNGStream(). |
| development/adr/v05-seed-contract.md | Adds the accepted ADR defining the seed/RNG-stream contract. |
| development/plans/2026-07-06-seed-contract.md | Adds the implementation plan and task breakdown for the ADR. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+299
to
+302
| message( | ||
| "Falling back to the legacy generator for an unsupported v0.4 feature; ", | ||
| "for a given seed this produces different values than the v0.4 pipeline." | ||
| ) |
Comment on lines
+309
to
+312
| #' @param seed Optional whole-number seed. Generation uses an isolated | ||
| #' L'Ecuyer-CMRG sub-stream and restores the caller's RNG state and kind on | ||
| #' exit, so output is reproducible for a given seed and package version | ||
| #' without perturbing the caller's RNG. |
Comment on lines
+348
to
+351
| #' @param seed Optional whole-number seed. Generation uses an isolated | ||
| #' L'Ecuyer-CMRG sub-stream and restores the caller's RNG state and kind on | ||
| #' exit, so output is reproducible for a given seed and package version | ||
| #' without perturbing the caller's RNG. |
Five-agent PR review findings. Suppress the RNGkind restore warning in production (previously only silenced in tests), validate the stage name before the seed early-return, and guard the legacy loop against a silent empty result. Pin the frozen .MOCK_STAGES indices and a postprocess-stage value so a future stage insertion can't silently shift seeded output. Add coverage for the always-on fallback message and simstudy isolation. Scope the NEWS 'all randomness' claim to the create_mock_data() pipeline (standalone create_* helpers are unchanged), fix a stale 'set global seed' roxygen bullet, and correct stale seed/seed+1L prose and a render-false worked example in the migrating and recodeflow vignettes.
Collaborator
Author
|
Ran the multi-agent PR-review skill (code, tests, silent-failure, comments, docs). The mechanism verified correct (independent devtools::test + R CMD check clean), but the review found real defects the per-task/whole-branch gates missed. Fixed in 0f9bda8: Correctness/user-facing
Test coverage
Docs
Deferred to follow-up: README hardcoded-output refresh (#49), version bump (release-prep when v0.5 tags with #39). Suite: FAIL 0 | WARN 35 | SKIP 11 | PASS 699. R CMD check: 0/0, environmental NOTEs only. |
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.
Phase 2, first workstream of the post-v0.4.0 plan. Implements the accepted seed-contract ADR. Co-releases with #39 (formula evaluator) — do not tag v0.5 on this alone. This is the single budgeted seeded-output break for v0.5.
What changes
.with_mock_seed()becomes the one seeding primitive across every generation path:.Random.seedandRNGkind(all three components), so generation never perturbs the caller's RNG on any seeded path — previously the legacyvalidate = FALSEpath clobbered it via a bareset.seed().seed, then advances to a named stage's provably-independent sub-stream viaparallel::nextRNGStream()(base R — no new external dependency). Stage indices are frozen (baseline=0, postprocess=1;formula/correlatereserved for [v0.5] Formula-derived variables: ADR + phased implementation #39/Correlated variables via the simstudy backend (gated on seed ADR) #42) so future stages don't reshuffle existing output.seed/seed+1Lsplit; native, postprocess, simstudy, and the legacy loop all route through it.The break
Output for a given seed changes once (Mersenne-Twister → L'Ecuyer-CMRG). It is reproducible for a given seed and package version, and now independent of the session's ambient
RNGkind(). Not comparable across the v0.4→v0.5 boundary — see the NEWS migration note. cchsflow/chmsflow pin against a version, so the note is mandatory.Guarantees (ADR D1–D4)
RNGkind(); D4 always-on fallback message.Review history
set.seed(kind = "L'Ecuyer-CMRG")pinned only the primary generator, sornorm()and equal-probabilitysample()still leaked ambientnormal.kind/sample.kind, partially breaking the D3 guarantee. Fixed by pinning all three kind components (a no-op under R ≥ 3.6 defaults, so pinned values were not re-baselined) plus a regression test that variesnormal.kind/sample.kind.Tests / docs
tests/testthat/test-seed-contract.R: isolation (state + kind), determinism, ambient-RNGkind independence (primary, normal, and sample kinds), stage independence, pinned reference values. Fulfils Add seed reproducibility tests across mock-data generators #22.R CMD check: 0 errors, 0 warnings, 3 environmental NOTEs.@param seeddocs across the three generators.Implements #38. Fulfils #22. Resolves #33 item 11. Follow-up: #47 (route standalone
create_*/survival generators through the primitive for package-wide isolation).Includes the accepted ADR (
development/adr/v05-seed-contract.md) and the implementation plan (development/plans/2026-07-06-seed-contract.md).