Skip to content

BUG: seed Monte Carlo per simulation index, not per worker (#1053)#1071

Open
wuisabel-gif wants to merge 2 commits into
RocketPy-Team:developfrom
wuisabel-gif:bug/monte-carlo-per-simulation-seed
Open

BUG: seed Monte Carlo per simulation index, not per worker (#1053)#1071
wuisabel-gif wants to merge 2 commits into
RocketPy-Team:developfrom
wuisabel-gif:bug/monte-carlo-per-simulation-seed

Conversation

@wuisabel-gif

Copy link
Copy Markdown

Pull request type

  • Code changes (bugfix, features)

Checklist

  • Tests for the changes have been added
  • Lint passes locally (ruff check and ruff format --check; pylint 10/10 on the touched files)
  • Affected unit suites pass locally (test_monte_carlo.py, tests/unit/stochastic); relying on CI for the full slow suite
  • CHANGELOG.md updated

Current behavior

MonteCarlo.simulate() seeds the stochastic models per worker in parallel mode and effectively once at construction in serial mode, so the inputs a given simulation index receives depend on how the run was executed rather than on the index. Parallel spawns one seed per worker from a fresh unseeded SeedSequence, and each worker draws for whatever sims it pulls off the shared counter; serial draws one long sequential stream. Result: serial, n_workers=2, and n_workers=4 all sample different inputs, and parallel runs aren't reproducible run to run. This is #1053.

New behavior

Each simulation's seed is derived from (master seed, simulation index) via SeedSequence, reseeding the models before every simulation in both paths. Adds an optional seed to MonteCarlo; the master entropy is captured once, so seed=None stays random but consistent across execution modes within a run. A given index now samples identical inputs regardless of serial/parallel or n_workers, and a fixed seed reproduces a whole run. Integer child seeds (not SeedSequence objects) keep custom samplers' reset_seed working.

Breaking change

  • No

With seed=None the sampling is unchanged in spirit (still random), but numeric samples for existing unseeded runs will differ, since seeding is now per-index rather than one sequential stream. The seed argument is opt-in.

Additional information

Adds test_seed_makes_inputs_reproducible_by_index and test_unseeded_monte_carlo_is_index_consistent in tests/unit/simulation/test_monte_carlo.py.

Closes #1053

@wuisabel-gif
wuisabel-gif requested a review from a team as a code owner July 19, 2026 03:33
@Gui-FernandesBR

Copy link
Copy Markdown
Member

duplicated with #1053

@codecov

codecov Bot commented Jul 19, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 76.92308% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.22%. Comparing base (ce3342f) to head (a0b9536).
⚠️ Report is 4 commits behind head on develop.

Files with missing lines Patch % Lines
rocketpy/simulation/monte_carlo.py 76.92% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1071      +/-   ##
===========================================
+ Coverage    80.85%   82.22%   +1.37%     
===========================================
  Files          122      122              
  Lines        16355    16361       +6     
===========================================
+ Hits         13224    13453     +229     
+ Misses        3131     2908     -223     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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