Skip to content

BUG: build Monte Carlo flights with the configuration they were given - #1164

Merged
Gui-FernandesBR merged 1 commit into
RocketPy-Team:developfrom
thc1006:bug/monte-carlo-keeps-the-flight-configuration
Aug 15, 2026
Merged

BUG: build Monte Carlo flights with the configuration they were given#1164
Gui-FernandesBR merged 1 commit into
RocketPy-Team:developfrom
thc1006:bug/monte-carlo-keeps-the-flight-configuration

Conversation

@thc1006

@thc1006 thc1006 commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Pull request type

  • Code changes (bugfix, features)

Checklist

  • Tests for the changes have been added (if needed)
  • Docs have been reviewed and added / updated
  • Lint (ruff check / ruff format --check, pylint) has passed locally
  • All tests (pytest tests/unit tests/integration) have passed locally

Current behavior

MonteCarlo.__run_single_simulation writes the Flight constructor out by hand and stops at time_overshoot. StochasticFlight.create_object passes nine arguments it does not:

max_time, max_time_step, min_time_step, rtol, atol, name, equations_of_motion, ode_solver, simulation_mode.

Counted on develop: 8 keyword arguments in the Monte Carlo path against 17 in StochasticFlight.create_object.

So a Monte Carlo run ignores the max time, the tolerances, the solver and the equations of motion the caller set, and resets the simulation mode to the constructor default. The same rocket, environment and flight fly differently under MonteCarlo than under StochasticFlight.create_object, which is the opposite of what #1070 added StochasticFlight's handling of these for.

New behavior

The Monte Carlo path passes the same seventeen, read the same way StochasticFlight.create_object reads them: max_time off the stochastic model, the rest off the flight it wraps.

Breaking change

  • No

Runs that set any of these were silently getting the defaults, so their results change. That was the bug.

Additional information

One test builds the flight through __run_single_simulation with a stand-in Flight and asserts each of the nine arrives. Dropping max_time, the solver, or the pair of tolerances turns it red.

Local run against develop at 1d04bcc: ruff clean, pylint exit 0, pytest tests/unit tests/integration 2214 passed, 51 skipped.

#1126 is open against this same method, for the separate matter of how the launch fields are sampled. This change only appends to the argument list and leaves those lines alone, but whichever lands second will want a look.

@thc1006
thc1006 requested a review from a team as a code owner August 14, 2026 21:29
@codecov

codecov Bot commented Aug 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.33%. Comparing base (f604534) to head (0f4b33f).
⚠️ Report is 2 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1164   +/-   ##
========================================
  Coverage    84.32%   84.33%           
========================================
  Files          130      130           
  Lines        17258    17258           
========================================
+ Hits         14553    14554    +1     
+ Misses        2705     2704    -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.

@Gui-FernandesBR Gui-FernandesBR linked an issue Aug 15, 2026 that may be closed by this pull request
__run_single_simulation writes the Flight constructor out by hand and stops at
time_overshoot. StochasticFlight.create_object passes nine more: max_time, the
two time steps, rtol, atol, name, equations_of_motion, ode_solver and
simulation_mode.

So a Monte Carlo run ignored the max time, the tolerances, the solver and the
equations of motion the caller had set, and reset the simulation mode to the
constructor default. The same rocket, environment and flight therefore flew
differently under MonteCarlo than under StochasticFlight.create_object. RocketPy-Team#1070
added StochasticFlight's handling of these; this path never picked it up.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
@Gui-FernandesBR
Gui-FernandesBR force-pushed the bug/monte-carlo-keeps-the-flight-configuration branch from db507c4 to 0f4b33f Compare August 15, 2026 02:28
@Gui-FernandesBR
Gui-FernandesBR merged commit 76cab5a into RocketPy-Team:develop Aug 15, 2026
8 checks passed
@thc1006
thc1006 deleted the bug/monte-carlo-keeps-the-flight-configuration branch August 15, 2026 07:39
Gui-FernandesBR added a commit to thatrandomasiandev/RocketPy that referenced this pull request Aug 15, 2026
Conflict was one test file: both sides appended to the end of
tests/unit/stochastic/test_stochastic_flight.py, develop's RocketPy-Team#1109 tests for
initial_solution and this branch's RocketPy-Team#1090 regressions. Kept both.

One real interaction beyond the conflict. RocketPy-Team#1164, merged today, added
test_a_monte_carlo_flight_keeps_the_configuration_it_was_given, which stubs
self.flight with a types.SimpleNamespace carrying _randomize_rail_length,
_randomize_inclination and _randomize_heading. __run_single_simulation now
asks for _sample_flight_inputs instead, so the stub raised AttributeError.
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.
That ties RocketPy-Team#1164's configuration test to RocketPy-Team#1090's invariant rather than
leaving the sampled values unchecked.

Confirmed the bug this fixes is a data-integrity one, not just wasted draws.
On develop, __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 --
which is what gets written to the inputs file -- held only the third. With
spreads on all three inputs and seed 4242:

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

so every exported 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.
Only heading agreed, because it happened to be drawn last. After this
change all three match.

Also answered my own question on the PR about parallel mode: there is no
hazard. __run_in_parallel gives each worker its own spawned seed, so
consolidating three draws into one only reduces how many numbers a worker
consumes and cannot couple workers. Measured: a fixed seed reproduces its
draws, distinct seeds diverge, four worker seeds produce no repeated draw,
and last_rnd_dict equals the returned draw on every call.

Fixed-seed baselines do move, since a simulation now consumes one draw
instead of three. That is the fix, not a side effect, but it is worth a
changelog line.

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

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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