-
-
Notifications
You must be signed in to change notification settings - Fork 197
ENH: Improve parachute geometric parametrization #835
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
ENH: Improve parachute geometric parametrization #835
Conversation
There was a problem hiding this 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)
rocketpy/rocket/rocket.py
Outdated
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). |
There was a problem hiding this comment.
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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #835 +/- ##
===========================================
+ Coverage 80.02% 80.05% +0.03%
===========================================
Files 98 98
Lines 12004 12026 +22
===========================================
+ Hits 9606 9628 +22
Misses 2398 2398 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this 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
, andporosity
toParachute
andadd_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 |
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
rocketpy/rocket/parachute.py
Outdated
self.parachute_radius = parachute_radius | ||
if parachute_height is None: | ||
parachute_height = parachute_radius | ||
self.parachute_height = parachute_height |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cleaner
self.parachute_radius = parachute_radius | |
if parachute_height is None: | |
parachute_height = parachute_radius | |
self.parachute_height = parachute_height | |
self.parachute_radius = parachute_radius | |
self.parachute_height = parachute_height or parachute_radius |
rocketpy/simulation/flight.py
Outdated
|
||
# Get the mass of the rocket | ||
mp = self.rocket.dry_mass | ||
|
||
# Define constants | ||
ka = 1 # Added mass coefficient (depends on parachute's porosity) | ||
R = 1.5 # Parachute radius | ||
ka = 1.068 * (1 - 1.465 * porosity - 0.25975 * porosity**2 + 1.2626 * porosity**3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this ka
property should be saved inside the Parachute class
rocketpy/simulation/flight.py
Outdated
# Get Parachute data | ||
cd_s = self.parachute_cd_s | ||
parachute_radius = self.parachute_radius | ||
parachute_height = self.parachute_height | ||
porosity = self.parachute_porosity | ||
|
There was a problem hiding this comment.
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
pseudo_drag = -0.5 * rho * cd_s * free_stream_speed | ||
# pseudo_drag = pseudo_drag - ka * rho * 4 * np.pi * (R**2) * Rdot |
There was a problem hiding this comment.
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 |
@@ -154,13 +157,27 @@ def __init__( | |||
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 |
There was a problem hiding this comment.
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.
@@ -261,6 +261,9 @@ main = calisto.add_parachute( | |||
sampling_rate=105, | |||
lag=1.5, | |||
noise=(0, 8.3, 0.5), | |||
parachute_radius=1.5, | |||
parachute_height=1.5, | |||
porosity=0.0432, |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
or
Parachute.parachute_height : float | |
Parachute.parachute_height : Optional[float] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: how should the Monte Carlo simulations be affected by this PR?
Should we open another PR allowing the user to vary these new arguments within the StochasticParachute
(https://docs.rocketpy.org/en/latest/reference/classes/monte_carlo/stochastic_models/stochastic_parachute.html) class?
if parachute_height is None: | ||
parachute_height = parachute_radius | ||
self.parachute_height = parachute_height |
There was a problem hiding this comment.
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
Pull request type
Checklist
black rocketpy/ tests/
) has passed locallypytest tests -m slow --runslow
) have passed locallyCHANGELOG.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