BUG: seed parachute pressure noise with per-instance RNG (#1091) - #1134
Merged
Gui-FernandesBR merged 1 commit intoAug 14, 2026
Merged
Gui-FernandesBR merged 1 commit into
Gui-FernandesBR merged 1 commit into
Conversation
Gui-FernandesBR
force-pushed
the
bug/1091-parachute-noise-seed
branch
from
August 12, 2026 22:40
e43b8a8 to
36f9d6b
Compare
Gui-FernandesBR
approved these changes
Aug 12, 2026
Gui-FernandesBR
force-pushed
the
bug/1091-parachute-noise-seed
branch
from
August 14, 2026 00:11
36f9d6b to
469b750
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1134 +/- ##
===========================================
+ Coverage 82.18% 83.29% +1.11%
===========================================
Files 122 130 +8
Lines 16355 17080 +725
===========================================
+ Hits 13441 14227 +786
+ Misses 2914 2853 -61 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
6 tasks done
thc1006
added a commit
to thc1006/RocketPy
that referenced
this pull request
Aug 16, 2026
StochasticParachute.create_object derives the pressure noise seed after the draw, and RocketPy-Team#1134 relied on last_rnd_dict being the same dictionary to carry it into the record. Snapshotting the draw broke that link: the parachute is still built with the seed, but the record loses it, so the Monte Carlo inputs stop describing the parachute that flew. develop recorded 37773913418288439290323614982376424810 before recorded <absent> The source scan missed it because it only read dict_generator overrides. It reads create_object too now, and tracks the names a method binds from a draw rather than guessing at a variable name, so a local a method fills in for its own use is not mistaken for a record. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
thc1006
added a commit
to thc1006/RocketPy
that referenced
this pull request
Aug 25, 2026
StochasticParachute.create_object derives the pressure noise seed after the draw, and RocketPy-Team#1134 relied on last_rnd_dict being the same dictionary to carry it into the record. Snapshotting the draw broke that link: the parachute is still built with the seed, but the record loses it, so the Monte Carlo inputs stop describing the parachute that flew. develop recorded 37773913418288439290323614982376424810 before recorded <absent> The source scan missed it because it only read dict_generator overrides. It reads create_object too now, and tracks the names a method binds from a draw rather than guessing at a variable name, so a local a method fills in for its own use is not mistaken for a record. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Gui-FernandesBR
pushed a commit
that referenced
this pull request
Sep 9, 2026
* BUG: sample around the nominal a stochastic model was built with _set_stochastic re-validates every declared input, and validation reads the nominal off the wrapped object. create_object writes the sampled value back onto that same object on purpose, so re-reading it on a reseed took one simulation's output as the next one's nominal: a wind factor compounded 10 -> 8.576 -> 7.355 -> 6.308 under a single fixed seed, and a plain scalar spec drifted the same way. Read the nominal once and keep it. Containers are copied on the way in, so writing through the wrapped object cannot reach it either. A component position arrives through an injected getter, reads an attribute nothing writes back to, and shares one name across every component, so those are read live rather than cached. Extracted from #1054. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * BUG: keep the nominal out of reach of what is generated from it Copying on the way in was not enough. _nominal handed back the kept object itself, and on the empty-spec path that one object reached the model attribute, last_rnd_dict and the FreeFormFins create_object returns, so a write through any of them moved what the next reseed sampled around. _snapshot_of stopped at a tuple as well, which left an array inside an airfoil pair shared with the object it came from. Copy on the way out too, and recurse through the built-in containers. The documented contract now names the four cases that stay outside it: an input added after construction, a component position, an ensemble wind factor, and anything that is not an array or a built-in container. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * TST: pin when a late input is captured, and the spread-tuple path An add_* input is configured after __init__, so its nominal is read then. The new test writes the rocket's eccentricity before add_cp_eccentricity and again after it, and only the first one may reach the draw. The (std, distribution) form now runs its own seed histories rather than repeating one seed, which is what a cache keyed by the seed instead of by the model actually fails. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * TST: pin the Function nominal as a boundary, not a footnote The documented exception said a Function is held as it was given. Nothing enforced it, so closing the hole later would have gone unnoticed and the documentation would have quietly become wrong. Measured: set_source on the rocket's drag curve moves the drawn value from 0.377 to 0.890, and deepcopy of that curve costs 6 microseconds. Cost is not the reason to leave it. _snapshot_of cannot raise today, and deepcopying whatever a user passed, on a path that runs on every reseed, would make it able to. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * BUG: read the nominal again when a late input is configured again Keeping the nominal gave the second add_cp_eccentricity nothing to replace, so it went on sampling around the value the rocket held at the first call: 0.5 where 0.8 was asked for. Reproducible, and around the wrong centre, which is harder to notice than a value that moves. Late configuration drops the kept nominal before validation reads one, and puts it back if validation raises, so a refused call leaves the previous configuration standing. Only the reconfiguration path. Passing None still leaves the earlier declaration in place, which is develop's behaviour and not this branch's. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * DOC: say what the snapshot does not do Measured on the current implementation: a cycle recurses until Python stops it, two references to one list come back as two lists, and the elements of an object-dtype array stay shared. None of those reach a supported nominal, but the docstring read like a general deep copy and should not. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * BUG: take a late input away when it is configured to None None is a configuration too. The nominal was refreshed but the earlier distribution stayed declared, so the next reseed validated it again and drew an uncertainty the caller had asked to remove. Filed as #1171 while the removal lived elsewhere; it belongs in the replacement helper this branch added, so it is here rather than in a second PR that owns the other half of one state transition. None still means an axis that was never given, and removing what was never declared stays a no-op. Both meanings have a test. The snapshot test asserted a dict entry was not None, which held whether or not anything had been copied, and no test reached the set branch at all. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * BUG: replace a pair of late inputs together or not at all add_cp_eccentricity takes x and y in one call, so a y that will not validate left x already replaced and declared. Validation happens for the whole group before anything is committed now. The test gives only y first, so x is undeclared going in and a partial commit shows up as an eccentricity the caller never successfully asked for. Asserting the nominal alone did not catch it, since x's nominal was restored either way. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * BUG: give every draw its own copy of a mutable value Copying on the way out of the kept nominal was not the last boundary. The list branch handed back the candidate itself, which for an empty spec is the model's own working value, and FreeFormFins keeps shape_points by reference. Writing through the first generated fins reached the second ones: first = stochastic.create_object() first.shape_points[1] = (9.9, 9.9) second = stochastic.create_object() # (9.9, 9.9) as well No reseed in between, which is how create_object is documented to be used and how a serial Monte Carlo runs it. last_rnd_dict was the same dictionary the values were built from, so it moved with them too. It records what was drawn now, which matters because a Monte Carlo writes it out after the flight rather than before. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * BUG: record a draw after the subclass that adjusts it, not before Recording in the base generator put the record before StochasticFreeFormFins had pulled the fin root back onto the body line. Under seed 7 the two root points drifted to 0.000299 and 0.001340, the correction returned them to zero, and the record kept the outline the fins were never built from. The rocket copies each component's record into its own, so the Monte Carlo input log carried it too. That is the failure class #1090 was about, arriving from the other side. _record_draw is the one place a model publishes what it drew, and a subclass that changes a value calls it again. A source scan holds the next subclass to the same rule, since the one that gets it wrong is the one nobody wrote a fixture for. _declare_stochastic_input and the _MISSING sentinel had no callers left after the grouped reconfiguration landed, and the first still carried the None handling that #1171 was about, so they are gone rather than left as a second lifecycle for someone to reach for. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * DOC: _choose returns a copy, and say so where it is documented The docstring still promised values itself when there are no candidates, which stopped being true when the draw started handing back a copy. Nothing reaches that branch through a validated input, since an empty list validates to the object's own value, so it is a guard against integers(0) rather than a path with a caller. It has a test now, which is also the one line of this change Codecov had no coverage for. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * BUG: record the parachute noise seed the parachute was built with StochasticParachute.create_object derives the pressure noise seed after the draw, and #1134 relied on last_rnd_dict being the same dictionary to carry it into the record. Snapshotting the draw broke that link: the parachute is still built with the seed, but the record loses it, so the Monte Carlo inputs stop describing the parachute that flew. develop recorded 37773913418288439290323614982376424810 before recorded <absent> The source scan missed it because it only read dict_generator overrides. It reads create_object too now, and tracks the names a method binds from a draw rather than guessing at a variable name, so a local a method fills in for its own use is not mistaken for a record. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * TST: walk the subclass tree, not the package exports StochasticMotorModel is a StochasticModel subclass that rocketpy.stochastic does not export, so the scan could not see it. It overrides neither method today, which is why nothing was wrong, and which is also why the gap would have gone unnoticed until something did. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * BUG: stop an omitted axis from taking away what was declared Removing on None looked like one line inside the new replacement helper, and it is not. add_cp_eccentricity(x=..., y=...) defaults both to None, so an omitted axis and an explicit None read identically, and the removal took away an axis the caller never mentioned: add_cp_eccentricity(x=0.001, y=0.002) add_cp_eccentricity(x=0.005) # y quietly gone develop keeps y here, and so does this again. Removing an earlier declaration needs an argument omission cannot supply, which is a signature change and its own decision, so it stays in #1171 rather than arriving inside a change about nominal ownership. The test that asked for removal is replaced by one that holds the omitted axis in place, since that is the behaviour anything already written depends on. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * DOC: say what a second add_cp_eccentricity call does Both arguments read as optional and nothing said what happens when the method is called again, which is the whole of the question behind #1171. Each public docstring now states it: a later call replaces what was configured, an omitted axis keeps what it had, and taking one away is not supported. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * BUG: leave an axis that was left out entirely alone Keeping its declaration was not enough. The omitted axis still went through the whole replacement: its kept nominal was dropped, None was validated again into a lone nominal, and that was written back over its distribution. The private side then said the axis was random while the attribute dict_generator reads said it was not, so it stopped varying: add_cp_eccentricity(x=0.001, y=0.002) add_cp_eccentricity(x=0.005) eight draws of y -> one distinct value A serial Monte Carlo never resets, so a whole study would have run with that axis switched off and nothing raised. Dropping the nominal also moved the centre. With the rocket's own y changed between the two calls, the next reset centred the old distribution on 9.0 rather than the 0.0 it was configured around. An axis given as None that already has a configuration is now left out of the transaction: not revalidated, its nominal not re-read, its attribute not rewritten. The test covers both eccentricity methods and looks before the reset as well as after, which is where the previous one missed it. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> * DOC: say what a snapshot does not reach inside an object array ndarray.copy() copies an object array without copying its entries, so a later write through one of them is still visible. The scope was numeric arrays already; this says so. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --------- Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Parachutean optionalseedand a per-instancenumpy.random.Generatorso pressure noise no longer draws from the process-global RNG (Fixes #1091).StochasticParachuteinto created parachutes when Monte Carlo seeds the model.Test plan
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 PYTHONPATH=. pytest tests/unit/rocket/test_parachute_noise_seed.py tests/unit/rocket/test_parachute.py tests/unit/stochastic/test_stochastic_parachute.py