Skip to content

Conversation

@ArthurJWH
Copy link
Contributor

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

  • Yes
  • No

@ArthurJWH ArthurJWH requested a review from a team as a code owner June 26, 2025 01:04
@ArthurJWH ArthurJWH changed the base branch from master to develop June 26, 2025 01:05
Copy link

Copilot AI left a comment

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).
Copy link
Member

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)

@github-project-automation github-project-automation bot moved this from Backlog to Next Version in LibDev Roadmap Jul 4, 2025
@codecov
Copy link

codecov bot commented Jul 15, 2025

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.

@Gui-FernandesBR Gui-FernandesBR requested a review from Copilot July 16, 2025 01:09
Copy link

Copilot AI left a comment

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
Copy link

Copilot AI Jul 16, 2025

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.

Suggested change
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.
@ArthurJWH ArthurJWH self-assigned this Jul 19, 2025
Copy link
Member

@Gui-FernandesBR Gui-FernandesBR left a comment

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

Copy link
Member

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
Copy link
Member

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?

Suggested change
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
Copy link
Member

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,
Copy link
Member

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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Parachute.parachute_height : float
Parachute.parachute_height : float, None

or

Suggested change
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
Copy link
Member

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

@Gui-FernandesBR Gui-FernandesBR linked an issue Jul 26, 2025 that may be closed by this pull request
Comment on lines 1443 to 1444
parachute_radius=1.5,
parachute_height=None,
Copy link
Member

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.

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

Copy link
Contributor Author

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?

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Member

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
)
Copy link
Member

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

Copy link
Member

Choose a reason for hiding this comment

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

@Gui-FernandesBR
Copy link
Member

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

@ArthurJWH
Copy link
Contributor Author

@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
)
Copy link
Member

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

Copy link
Contributor Author

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?

Copy link
Member

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

Copy link
Member

@Gui-FernandesBR Gui-FernandesBR left a comment

Choose a reason for hiding this comment

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

Best PR o 2025 so far, congratulations, @ArthurJWH !!

@MateusStano MateusStano merged commit 0dbc808 into develop Sep 15, 2025
10 checks passed
@MateusStano MateusStano deleted the enh/improve-parachute-geometric-parametrization branch September 15, 2025 20:06
@github-project-automation github-project-automation bot moved this from Next Version to Closed in LibDev Roadmap Sep 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Closed

Development

Successfully merging this pull request may close these issues.

ENH: Improve Parachute Input Values

3 participants