BUG: sample StochasticFlight inputs once per simulation (#1090)

[thatrandomasiandev]

Pull request type

• Code changes (bug fix, features)

Checklist

• Tests for the changes have been added (if needed)
• Docs have been reviewed and added / updated
• Lint (black rocketpy/ tests/ / make lint) has passed locally
• All tests (pytest tests -m slow --runslow) have passed locally
• CHANGELOG.md — no action needed; an LLM workflow auto-updates it after merge

Current behavior

MonteCarlo.__run_single_simulation called _randomize_rail_length, _randomize_inclination, and _randomize_heading, each of which invoked next(self.dict_generator()). One simulation therefore drew three times; the Flight mixed values across draws while last_rnd_dict (and .inputs.txt) kept only the last draw. StochasticFlight.create_object had the same pattern for rail_length.

Fixes #1090

New behavior

StochasticFlight._sample_flight_inputs() performs a single draw. create_object and MonteCarlo.__run_single_simulation both use that helper so flown rail/inclination/heading match last_rnd_dict. The _randomize_* helpers remain for single-field callers and still each draw once.

Breaking change

• No

Additional information

Focused unit tests assert create_object and __run_single_simulation Flight inputs equal last_rnd_dict. Full slow suite not run in this contribution pass.

[codecov]

Codecov Report

❌ Patch coverage is 70.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.57%. Comparing base (1b87124) to head (b2ba5f1).

Files with missing lines	Patch %	Lines
rocketpy/stochastic/stochastic_flight.py	50.00%	3 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #1126   +/-   ##
========================================
  Coverage    84.57%   84.57%           
========================================
  Files          131      131           
  Lines        17525    17527    +2     
========================================
+ Hits         14821    14824    +3     
+ Misses        2704     2703    -1     

☔ 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.

[ting-hong-shieh]

I compared the flight inputs at current develop (cb6106a717207dd8fc2dfe1446d80ff75022f21b) and head 02fcccb39fa58278ca055f22d0fe80183b9bddd8 with the same stochastic seed (4242).

// develop
{
  "flown":  {"heading": 53.55816157603624, "inclination": 81.41474397857291, "rail_length": 5.2},
  "logged": {"heading": 53.55816157603624, "inclination": 83.62213583873555, "rail_length": 5.2}
}

// PR head
{
  "flown":  {"heading": 52.78714695725011, "inclination": 85.16622077057734, "rail_length": 5.2},
  "logged": {"heading": 52.78714695725011, "inclination": 85.16622077057734, "rail_length": 5.2}
}

The absolute sampled values differ between implementations because the head intentionally makes one draw instead of three. The relevant invariant is that the values flown and recorded are identical within the same run; that fails on develop and holds on this head.

pytest tests/unit/stochastic/test_stochastic_flight.py -q → 5 passed. No branch changes were made.

Environment: Python 3.12.6; NumPy 2.5.2; pytest 9.1.1; macOS 26.5.2 arm64.

[Gui-FernandesBR]

I wonder whether this is changing the monte carlo behavior when we work with parallel mode

[Gui-FernandesBR]

This is now conflicting with develop — please merge develop in and push so CI runs against the current base.

Also still open from my earlier comment, and it is the part I actually want pinned down before merging: what does "sample once per simulation" mean under parallel mode? __sim_producer reseeds each worker with _set_stochastic(seed) and then calls create_object() per iteration, so I want to be sure that a single draw per simulation does not make two workers walk the same sequence, or make the run non-reproducible for a given seed. Please describe the behaviour explicitly and, ideally, add a test that pins it (same seed twice gives the same sequence of draws; two workers do not collide).

[Gui-FernandesBR]

Merged develop in and pushed the remaining fix to your branch (b2ba5f11). All 9 checks green, mergeable.

Answering my own earlier question about parallel mode: there is no hazard. __run_in_parallel gives each worker its own spawned seed and each worker reseeds itself, so collapsing three draws into one only changes how many numbers a worker consumes — it can't couple workers. Measured it rather than assuming: a fixed seed reproduces its draws, distinct seeds diverge, four worker seeds produce no repeated draw between them, and last_rnd_dict equalled the returned draw on every single call. That was the one thing holding this up from my side.

The bug is worse than the title suggests, and worth saying out loud in the changelog. It isn't just wasted draws — it's a data-integrity bug. __run_single_simulation called the three _randomize_* helpers and each ran its own next(dict_generator()), so a simulation flew the first draw's rail_length and the second draw's inclination, while last_rnd_dict — what gets written to the inputs file — held only the third. With spreads on all three inputs and seed 4242, on develop:

flown   rail_length=5.433110  inclination=83.622136  heading=54.525483
logged  rail_length=5.526158  inclination=86.327132  heading=54.525483

Every exported inputs row has carried a rail length off by 9.3 cm and an inclination off by 2.7 degrees from the flight that produced the outputs next to it. Only heading agreed, because it happened to be drawn last. Anyone who has correlated Monte Carlo outputs against the logged inputs has been regressing against values that were never flown. With your change all three match.

One real interaction I had to fix. #1164 merged today and added test_a_monte_carlo_flight_keeps_the_configuration_it_was_given, which stubs self.flight with a SimpleNamespace carrying the three _randomize_* methods. __run_single_simulation now asks for _sample_flight_inputs, so that stub raised AttributeError. I updated it to the single-draw API and, since the stub now hands over all three values in one dict, made the test also assert they reach the Flight — which ties #1164's configuration test to your invariant instead of leaving the sampled values unchecked.

Two notes, neither blocking:

• Fixed-seed baselines shift, because a simulation now consumes one draw instead of three. That's the fix rather than a side effect, but it deserves a changelog line so nobody reports it as a regression.
• _randomize_rail_length / _randomize_inclination / _randomize_heading now have no callers anywhere. They're private, so leaving them is harmless, but each still performs a full three-input draw and returns one key — so anyone calling them in sequence reproduces exactly the bug you just fixed. Worth deleting or documenting in a follow-up.
• The (apogee_ellipses, impact_ellipses) = → apogee_ellipses, impact_ellipses = edit in export_ellipses_to_kml is unrelated to MonteCarlo draws the flight dictionary three times, so the logged inputs are not the ones flown #1090. Harmless, just noting I saw it.

Verified before pushing: tests/unit 2163 passed / 17 skipped, ruff check and format clean, pylint 10.00/10.