ENH: Improve parachute geometric parametrization by ArthurJWH · Pull Request #835

ENH: Improve parachute geometric parametrization by ArthurJWH · Pull Request #835 · RocketPy-Team/RocketPy

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 (black rocketpy/ tests/) has passed locally
All tests (pytest tests -m slow --runslow) have passed locally
CHANGELOG.md has been updated (if relevant)

Current behavior

Parachute radius is assumed to be fixed (=1.5m)

New behavior

Other than adding a radius parameter, there is also additional height and porosity parameters.
The default is still set to the current behavior.

Breaking change

ArthurJWH changed the base branch from master to develop

June 26, 2025 01:05

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR improves the parachute geometric parametrization by introducing new parameters (radius, height, and porosity) to provide a more flexible and accurate simulation. Key changes include:

Adding lambda callbacks to update parachute_radius, parachute_height, and parachute_porosity in the simulation.
Updating the added mass computation in flight dynamics to use parachute_radius and parachute_height.
Extending the Parachute class to support the new parameters, with appropriate defaults and serialization.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
rocketpy/simulation/flight.py	Updated simulation callbacks and mass computation with new parameters.
rocketpy/rocket/parachute.py	Extended the Parachute class to include and serialize new parameters.

Comments suppressed due to low confidence (2)

rocketpy/simulation/flight.py:2002

[nitpick] Consider adding a comment to explain the rationale behind the modified added mass formula using parachute_radius squared times parachute_height, clarifying the geometrical model or assumptions used.

        ma = ka * rho * (4 / 3) * np.pi * parachute_radius**2 * parachute_height

rocketpy/simulation/flight.py:2018

Ensure that the variable 'z' is defined in this scope (for example, by extracting altitude from the state vector) to avoid potential runtime errors.

        az = (Dz - mp * self.env.gravity.get_value_opt(z)) / (mp + ma)

This comment was marked as outdated.

Comment on lines +1511 to +1514

		porosity : float, optional
		Porosity of the parachute material, which is a measure of how much air can
		pass through the parachute material.
		Default value is 0.0432 (for consistency with previous versions).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you define more precisely what porosity is? Something like: "The ratio of the area of the porous structure to the total area of the canopy" (I don't know if this is the correct definition in our case, just an example)

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.04%. Comparing base (f17893b) to head (486a5d1).
⚠️ Report is 22 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #835      +/-   ##
===========================================
+ Coverage    80.02%   80.04%   +0.02%     
===========================================
  Files           98      103       +5     
  Lines        12004    12529     +525     
===========================================
+ Hits          9606    10029     +423     
- Misses        2398     2500     +102

☔ View full report in Codecov by Sentry.
📢 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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enhances the parachute model by introducing geometric parameters (radius, height) and porosity, updating simulation calculations accordingly, and aligning tests and documentation.

Added parachute_radius, parachute_height, and porosity to Parachute and add_parachute API
Updated u_dot_parachute to compute added mass and gravity using the new parameters
Updated tests and user guides to include the new parameters

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
rocketpy/rocket/parachute.py	Added radius, height, and porosity attributes & serialization
rocketpy/rocket/rocket.py	Extended `add_parachute` signature and docstrings
rocketpy/simulation/flight.py	Incorporated geometry and porosity into mass & force calculations
tests/unit/test_utilities.py	Updated expected safety_factor for infinite Mach
tests/unit/test_flight.py	Updated expected final time in aerodynamic test
docs/user/rocket/rocket_usage.rst	Added new parameters to usage examples
docs/user/first_simulation.rst	Added new parameters to example
README.md	Added new parameters to README example

Comments suppressed due to low confidence (1)

rocketpy/simulation/flight.py:1999

The new parachute_radius, parachute_height, and porosity parameters are not explicitly covered by existing tests. Consider adding unit tests that verify how changes in these parameters affect added mass and drag force calculations in u_dot_parachute.

        porosity = self.parachute_porosity


		# Calculate added mass
		ma = ka * rho * (4 / 3) * np.pi * R**3
		ma = ka * rho * (4 / 3) * np.pi * parachute_radius*2 parachute_height

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The added mass calculation uses the full ellipsoid volume (4/3·π·a²·c) but the parachute is modeled as a hemispheroid; this should use half the volume (2/3·π·a²·c) to correctly compute added mass.

	ma = ka * rho * (4 / 3) * np.pi * parachute_radius*2 parachute_height
	ma = ka * rho * (2 / 3) * np.pi * parachute_radius*2 parachute_height

Copilot uses AI. Check for mistakes.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall code looks cleaner and better, congratulations on this implementation, @ArthurJWH .

I have a few comments that I believe would help you to improve clarity of your code. Please let me know if you have any questions.

Comment on lines +1980 to +1985

		# Get Parachute data
		cd_s = self.parachute_cd_s
		parachute_radius = self.parachute_radius
		parachute_height = self.parachute_height
		porosity = self.parachute_porosity

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need these definitions...

We can save a few nanoseconds by not defining them here.

You can use the attributes directly: using self.cds instead of cds

Comment on lines 2011 to 2012

		pseudo_drag = -0.5 * rho * cd_s * free_stream_speed
		# pseudo_drag = pseudo_drag - ka * rho * 4 * np.pi * (R*2) Rdot

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These comments??? Should we start using it?

	pseudo_drag = -0.5 * rho * cd_s * free_stream_speed
	# pseudo_drag = pseudo_drag - ka * rho * 4 * np.pi * (R*2) Rdot
	pseudo_drag = -0.5 * rho * cd_s * free_stream_speed
	# pseudo_drag = pseudo_drag - ka * rho * 4 * np.pi * (R*2) Rdot

		The values are used to add noise to the pressure signal which is
		passed to the trigger function. Default value is ``(0, 0, 0)``.
		Units are in Pa.
		parachute_radius : float, optional

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you be more spacific on the parachute_radius definition please? I believe it is still not intuitive what the radius is.

		noise=(0, 8.3, 0.5),
		parachute_radius=1.5,
		parachute_height=1.5,
		porosity=0.0432,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

having both cds and parachute_radius? do you think we could possible calculate the cd based on these other parameters?

		Parachute.parachute_radius : float
		Length of the non-unique semi-axis (radius) of the inflated hemispheroid
		parachute in meters.
		Parachute.parachute_height : float

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

	Parachute.parachute_height : float
	Parachute.parachute_height : float, None

	Parachute.parachute_height : float
	Parachute.parachute_height : Optional[float]

Comment on lines +197 to +199

		if parachute_height is None:
		parachute_height = parachute_radius
		self.parachute_height = parachute_height

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.parachute_height = parachute_height or parachute_radius

Comment on lines +1443 to +1444

		parachute_radius=1.5,
		parachute_height=None,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed it now...

Since it is the Parachute class, there's no need to specify this is the parachute radius or the parachute height.

	parachute_radius=1.5,
	parachute_height=None,
	radius=1.5,
	height=None,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was looking in other Rocket class methods such as add_trapezoidal_fins and set_rail_buttons, and both use radius already to refer to the fuselage radius. So changing it in the add_parachute method wouldn't break the consistency?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, since we already use radius in some inputs as the fuselage radius, maybe it will be more clear as an input to explicitly mention parachute

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to hear a third opinion here, maybe @MateusStano or @phmbressan .

Based on my experience, I don't think parachute_radius is a good idea, it sounds verbose to me.
However, I don't know iff we would have any conflict with other classes.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the simpler naming is better. If the documentation is there we can rely on that

Comment on lines +2014 to +2021

		ma = (
		self.ka
		* rho
		* (4 / 3)
		* np.pi
		* self.parachute_radius**2
		* self.parachute_height
		)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is calculating the volume of the whole sphere right? I think it should be divided by two to account for the hemispheroid shape only

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ArthurJWH please let me know when you have this PR ready for a re-review

@ArthurJWH please let me know when you have this PR ready for a re-review

I think it is ready for the review now

Comment on lines +203 to +208

		self.ka = 1.068 * (
		1
		- 1.465 * self.porosity
		- 0.25975 * self.porosity**2
		+ 1.2626 * self.porosity**3
		)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you document Parachute.ka in the class docstring please?

Also, what does ka mean? Could we think about a new, more descriptive name for it?

In python, we rather use the explicit version than implicit version of name variables.

self.aoa is not preferred
self.angle_of_attach is preferred

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ka is the added mass coefficient. Should I change the variable to added_mass_coefficient and document it?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please. To document it, you just have to add some docstrings to class Attributes section