BUG: fly the parachute the simulation sampled

[thc1006]

Closes #1094.

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 rocketpy/ tests/ docs/) has passed locally
• All tests have passed locally

pylint exits 0, and pytest tests/unit tests/integration is 2033 passed, 44 skipped. I did not run -m slow --runslow. No code under rocketpy/simulation/ changes, but the values this patch corrects are read by the flight path, so that is a statement about the diff and not about the blast radius. The breaking-change section below has the numbers.

Current behavior

StochasticParachute.create_object() builds a Parachute from a complete draw:

generated_dict = next(self.dict_generator())
return Parachute(**generated_dict)

StochasticRocket.create_object() then discards that object and builds a second one from six of its ten fields:

parachute = self._create_parachute(parachute)
rocket.add_parachute(
    name=parachute.name,
    cd_s=parachute.cd_s,
    trigger=parachute.trigger,
    sampling_rate=parachute.sampling_rate,
    lag=parachute.lag,
    noise=parachute.noise,
)

radius, height, porosity and drag_coefficient are not passed. Parachute.__init__ accepts all four and Rocket.add_parachute forwards all four, so the omission is silent: the second parachute re-derives radius from cd_s and the default drag coefficient, and height falls back to that radius.

Randomizing the geometry is not required to hit this, which is the part I had wrong when I filed the issue. A parachute configured with an explicit radius and no randomization at all:

                 configured   flown
radius           2.0          1.5193034332796236
height           1.5          1.5193034332796236
porosity         0.05         0.0432
drag_coefficient 1.4          1.4

radius and height come back equal because both were re-derived. So the affected set is every study whose parachute carries geometry, not only those that sample it, and last_rnd_dict records values the flight never used.

New behavior

The object _create_parachute already built is the one that gets attached:

rocket.parachutes.append(self._create_parachute(parachute))

Rocket.add_parachute does three things: construct, append, return. Nothing else is skipped by attaching directly. Same table after the change:

radius           2.0          2.0
height           1.5          1.5
porosity         0.05         0.05

This also stops Parachute.__init__ running twice per parachute per simulation. The second run drew the initial pressure noise from the global NumPy RNG again, which is #1091: fixing the seed tree without fixing this would leave the second initialization undoing the first.

Tests

Four in tests/unit/stochastic/test_stochastic_rocket.py, and each of the two failure modes is pinned by a mutation:

mutation	fails
rebuild through add_parachute, as today	3
build the parachute but never attach it	2

The overlap is deliberate. test_the_parachute_is_attached_exactly_once is the control: it survives the first mutation, because the old code did attach parachutes, and it catches the second, which would otherwise let a passing suite fly every Monte Carlo rocket with no parachutes at all.

Breaking change

• Yes

radius and height feed the added mass term in flight.py, so this is descent dynamics and not just bookkeeping:

ma = self.parachute_added_mass_coefficient * rho * (2 / 3) * np.pi \
     * self.parachute_radius**2 * self.parachute_height

For a parachute configured with radius=2.0, height=1.5 against the 1.5193 it was flying, added mass moves by about 71%. Landing points move with it. That is the bug being fixed, so the old numbers were not the right ones, but anyone with a baselined study will see them change.

One fewer draw from the global NumPy RNG per parachute per simulation, so anything downstream of that stream shifts too. #1091 is the reason that stream should not be there at all.

One case I would like your call on

There is a third group, and I got it wrong in the issue before measuring it.

If a parachute is built with no explicit geometry, Parachute.__init__ derives radius from cd_s, and StochasticParachute captures that derived value. The old code then re-derived it downstream from the sampled cd_s, so a study randomizing cd_s had its canopy grow and shrink with it. Attaching the sampled object stops that: radius now stays at the value derived from the nominal cd_s.

cd_s nominal 10.0, sampled 10.609

radius flown, before   1.5531   (from the sampled cd_s)
radius flown, after    1.5079   (from the nominal cd_s)
added mass                      -8.5%

Both are defensible. The old one is arguably more physical, since a larger drag area ought to mean a larger canopy. The new one is at least self-consistent, since the object now flies what it recorded, whereas before it recorded 1.5079 and flew 1.5531.

I did not try to keep the old coupling, because deriving geometry from a sampled cd_s is a modelling decision rather than a bug fix, and it belongs in StochasticParachute rather than here. If you would rather it were preserved, say so and I will follow up with it, either in this PR or a separate one.

Additional information

Found while reviewing #1054. The two overlap in rocketpy/stochastic/stochastic_rocket.py: #1054 changes the base model's list sampling and adds a component_collections skip, this one changes the parachute loop below it. They merge cleanly, and I ran the combined tree to check rather than assuming: tests/unit/stochastic and tests/unit/simulation are 275 passed, 4 skipped on the merge of the two heads. Whichever lands second still wants a rebase.

Related: #1091 (parachute pressure noise outside the seed tree), and #1093, #1095, #1096 from the same review.

[thc1006]

CI here is red on #1078 and not on anything in this branch. Three runs so far, all of them dying the same way:

Pytest (macos-latest, 3.14)   exit 139, in pyvista's plotter.render during
                              test_flight_animation_export_gif
everything else               cancelled by fail-fast, having found nothing

In the run that got furthest, the unit suite was 1855 passed and the doctests 46 passed before the integration job crashed. Locally the whole thing is green: pytest tests/unit tests/integration is 2033 passed, 44 skipped, and pylint rocketpy/ tests/ docs/ exits 0.

I have added the numbers to #1078. The short version is that it is a 3.14 fault rather than a platform one: nine failures on macOS 3.14 and five on ubuntu 3.14 across the last forty runs, the same stack on both, and nothing on 3.10 or on Windows.

Not asking anyone to chase that here. Flagging it so the red is not read as this branch.

[thc1006]

Two corrections to the description above, both mine and both now edited in.

I wrote that this touches no file #1054 changes. That is wrong: both change rocketpy/stochastic/stochastic_rocket.py. They merge cleanly and I ran the combined tree rather than assuming, tests/unit/stochastic and tests/unit/simulation are 275 passed and 4 skipped on the merge of the two heads, but the claim as written was false.

I also wrote that nothing here touches the flight path, in the same breath as a breaking-change section explaining that radius and height feed the added mass term. No code under rocketpy/simulation/ changes, which is what I meant, but that is a statement about the diff and not about what the change does.

[thc1006]

Thanks @Gui-FernandesBR. One check before this goes in, because the description has a question in it that an approval does not obviously answer, and I would rather ask twice than have it merge unnoticed.

The section is "One case I would like your call on". A parachute built without an explicit radius has one derived from cd_s. The old double construction re-derived it downstream from the sampled cd_s, so the canopy tracked the drag area. Attaching the sampled object stops that, and the radius stays at the value derived from the nominal cd_s. Measured on one parachute, that is about 8.5% on added mass, in the opposite direction to the 71% this PR fixes for explicitly configured geometry.

Both behaviours are defensible and I did not want to pick. If you are happy with the frozen-derived version, nothing needs to change and I will note it in the issue. If you would rather the derived case kept tracking cd_s, that is a change to StochasticParachute rather than to this loop, and I would send it separately rather than growing this one.

Rebased onto current develop a few minutes ago, so the run on it is post-#1084.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.55%. Comparing base (e0ff281) to head (17124df).
⚠️ Report is 20 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1098      +/-   ##
===========================================
+ Coverage    82.18%   82.55%   +0.37%     
===========================================
  Files          122      128       +6     
  Lines        16355    16555     +200     
===========================================
+ Hits         13441    13667     +226     
+ Misses        2914     2888      -26     

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

Take the frozen-derived version — nothing needs to change here.

The reason is that the geometry only feeds ma. Terminal velocity comes from cd_s alone (flight.py:2770), so the old coupling moved the opening transient, not the steady descent that sets the landing point. Running your numbers with mp ~ 16 kg: added mass +9.3%, but it is ~33% of mp + ma, so acceleration moves ~3% — against the ~43-71% error on explicitly configured geometry.

Your own argument settles it anyway: the object recorded 1.5079 and flew 1.5531. A run whose last_rnd_dict does not describe the flight cannot be audited. And if we ever want the coupling back, it belongs in StochasticParachute.create_object as a visible decision, not as a side effect of double construction. Anyone who wants it today can already correlate radius/height with cd_s via a custom sampler.

Please note it in the issue as you offered. Thanks for asking twice, and for the two corrections to the description — the stochastic_rocket.py overlap with #1054 was worth catching.