Repository navigation
BUG: fix Monte Carlo release blockers - #1213
Gui-FernandesBR wants to merge 1 commit into
Conversation
Addresses the Monte Carlo fixes in #1209. Simplifies study seeds and checkpoint recovery, preserves stochastic configurations, and adds regression coverage and migration notes.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1213 +/- ##
========================================
Coverage 91.66% 91.67%
========================================
Files 131 131
Lines 18240 18240
========================================
+ Hits 16720 16721 +1
+ Misses 1520 1519 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Gui-FernandesBR
left a comment
Gui-FernandesBR
left a comment
There was a problem hiding this comment.
Review notes on the Monte Carlo release fixes.
| seed : int, optional | ||
| Seed for the random number generator. | ||
| """ | ||
| self._seed = seed |
There was a problem hiding this comment.
_set_stochastic is also called internally by MonteCarlo.__seed_this_simulation (with a per-simulation integer) and by StochasticRocket._set_stochastic(None) (which spawns integer seeds for every child component even when seed=None). Recording self._seed = seed here causes a subsequent mc.simulate(..., append=False) without random_seed to emit a false object seeds are ignored warning, and makes StochasticParachute.create_object() reuse a fixed pressure-noise seed after stochastic_rocket._set_stochastic(None). Track user-configured seeds separately from internal reseeds.
| initial_solution = list(initial_solution.solution[-1]) | ||
| self._validate_initial_solution(initial_solution) | ||
| self.initial_solution = initial_solution | ||
| self.terminate_on_apogee = terminate_on_apogee |
There was a problem hiding this comment.
StochasticFlight(flight) drops flight.terminate_on_apogee when terminate_on_apogee is omitted (None), and create_object() / MonteCarlo.__run_single_simulation() also drop flight.use_udot_rail2. Both should be preserved from the nominal flight like max_time and time_overshoot.
| self.terminate_on_apogee = terminate_on_apogee | |
| self.terminate_on_apogee = ( | |
| flight.terminate_on_apogee | |
| if terminate_on_apogee is None | |
| else terminate_on_apogee | |
| ) |
| initial_solution = list(flight.solution[0]) | ||
| elif isinstance(initial_solution, Flight): | ||
| initial_solution = list(initial_solution.solution[-1]) | ||
| self._validate_initial_solution(initial_solution) |
There was a problem hiding this comment.
_validate_initial_solution checks isinstance(i, (int, float)), which rejects NumPy scalars (np.int64, np.float32) copied from flight.solution[0] and accepts bool. Validate elements with isinstance(i, Real) and not isinstance(i, (bool, np.bool_)).
| if terminate_on_apogee is not None and not isinstance( | ||
| terminate_on_apogee, bool | ||
| ): | ||
| raise AssertionError("`terminate_on_apogee` must be a boolean") |
There was a problem hiding this comment.
Combining the nested if put line 87 into the patch diff without a test, which is the 1 missed line reported by Codecov (83.33% patch coverage on this file). Either keep the original if lines untouched or add a unit test with an invalid terminate_on_apogee.
| strays = sorted( | ||
| held | ||
| - set(range(index_offset, len(found) + index_offset)) | ||
| - set(previous_indices) | ||
| ) |
There was a problem hiding this comment.
When previous_indices holds indices above target (e.g. {0, 10} with target=2), len(found) grows with len(previous_indices), so range(index_offset, len(found) + index_offset) hides stray indices between target and len(found).
| strays = sorted( | |
| held | |
| - set(range(index_offset, len(found) + index_offset)) | |
| - set(previous_indices) | |
| ) | |
| upper = target if previous_indices else max(target, len(found)) | |
| strays = sorted( | |
| held | |
| - set(range(index_offset, upper + index_offset)) | |
| - set(previous_indices) | |
| ) |
Fixes the Monte Carlo blockers in #1209: study seeds, checkpoint recovery, plots and stochastic configuration. Includes
simulate_convergence(random_seed=...)proposed in #1198 and documents the breaking changes. Local regression and serial/parallel replay tests, Ruff, pylint and the Sphinx build pass. The worker-pool and validate-once rewrites remain deferred to v1.15.