Repository navigation
BUG: fix Monte Carlo release blockers - #1213
Gui-FernandesBR wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1213 +/- ##
===========================================
+ Coverage 91.66% 91.68% +0.01%
===========================================
Files 131 131
Lines 18240 18245 +5
===========================================
+ Hits 16720 16728 +8
+ Misses 1520 1517 -3 ☔ 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.
There was a problem hiding this comment.
Fixed the false warning in b1e75cb: user-set seeds now live in a separate _configured_seed, and seeds the study or a parent rocket derive clear it, so a second simulate(..., append=False) without random_seed stays quiet (test in test_monte_carlo_seeding.py). The parachute part is not changed: after stochastic_rocket._set_stochastic(None) the parachute still keeps an integer pressure-noise seed. develop behaves the same, so I left it out of this PR.
| 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 | |
| ) |
There was a problem hiding this comment.
Done in b1e75cb. terminate_on_apogee falls back to flight.terminate_on_apogee, and use_udot_rail2 is passed through in both create_object() and MonteCarlo.__run_single_simulation, with tests for each.
| 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_)).
There was a problem hiding this comment.
Done in b1e75cb: elements are checked with isinstance(i, Real) and not isinstance(i, (bool, np.bool_)), so NumPy scalars pass and booleans are rejected.
| 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.
There was a problem hiding this comment.
Covered in b1e75cb with a unit test for an invalid terminate_on_apogee that hits the raise. I kept the combined if, since the nested form put pylint over its statement limit.
| 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) | |
| ) |
There was a problem hiding this comment.
Applied in b1e75cb: with previous_indices the upper bound is target, so {0, 10} with target=2 now flags index 3 as a stray. I also dropped "consecutive" from the error message.
Addresses the Monte Carlo fixes in #1209. Simplifies study seeds and checkpoint recovery, preserves stochastic configurations, and adds regression coverage and migration notes.
ee9e1ff to
271c53e
Compare
There was a problem hiding this comment.
I disagree with creating a TODO.md file again. Should be removed!
There was a problem hiding this comment.
I don't understand. Are we introducing any breaking changes with the next minor release or not?
There was a problem hiding this comment.
Yes. New serial studies number from 0 instead of 1, and a fixed random_seed gives different results than v1.13, because every simulation is reseeded and object seeds are ignored. The old ### Breaking changes section repeated the #1187 entry, so b1e75cb folds both points into that single ### Changed entry. Whether this ships as 1.14.0 or needs a major bump is up to us.
Keep the base flight's terminate_on_apogee and use_udot_rail2 in StochasticFlight and MonteCarlo, accept NumPy scalars in initial_solution, and stop a checkpointed log's length from hiding a stray index. Seeds a study derives for itself no longer count as object seeds, so a second study does not warn about them. Drop docs/TODO.md and fold the changelog's breaking-changes section into Changed. #1209 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XDW574nZTvpQsbhf7kAaFz
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.