-
-
Notifications
You must be signed in to change notification settings - Fork 232
ENH: Parachute Inflation Modeling #867
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?
Changes from all commits
c51beb0
c82edc9
e9077b9
c8005c6
3c40acc
a81a9a6
303491e
9db403a
05553d4
fec6bad
149ea9e
b9d02dd
17bcb39
8cef755
17b502f
3da56ff
523c5a6
9f28d6e
7334fc4
bfb3f37
a16a48c
7404daa
2f15b09
81667b5
4dac748
f1fa769
7aef262
c7dc21d
85fb3b8
301e08f
b18b9bf
b861b2d
7067104
0c75906
2d5d255
29fcfb4
5d8bb23
01bccda
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -765,6 +765,21 @@ def __simulate(self, verbose): | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "parachute_added_mass_coefficient", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| added_mass_coefficient, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| lambda self: setattr( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "parachute_volume", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| (4 / 3) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * math.pi | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * (self.parachute_height / self.parachute_radius) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| min( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.parachute_radius, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.rocket.radius, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * 3, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+771
to
+780
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| (4 / 3) | |
| * math.pi | |
| * (self.parachute_height / self.parachute_radius) | |
| * ( | |
| min( | |
| self.parachute_radius, | |
| self.rocket.radius, | |
| ) | |
| ) | |
| * 3, | |
| (2 / 3) | |
| * math.pi | |
| * self.parachute_radius ** 2 | |
| * self.parachute_height, |
Copilot
AI
Dec 14, 2025
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.
Attempting to delete the 't0' attribute before it has been set will raise an AttributeError. The 't0' attribute is only created later in u_dot_parachute (line 2774) using getattr with a default value. This callback should either be removed or moved to execute after the parachute phase ends, not when it begins.
| lambda self: delattr(self, "t0"), | |
| lambda self: delattr(self, "t0") if hasattr(self, "t0") else None, |
Copilot
AI
Dec 14, 2025
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 parachute_volume initialization formula appears to be incorrect. The expression simplifies to 4 * π * (height/radius) * min(radius, rocket.radius), which has units of m² (area), not m³ (volume). For a hemispheroid parachute, the initial volume should be approximately (2/3) * π * radius^2 * height (half of an oblate spheroid). The current formula will produce incorrect initial volume values that will affect the entire inflation simulation.
| (4 / 3) | |
| * math.pi | |
| * (self.parachute_height / self.parachute_radius) | |
| * ( | |
| min( | |
| self.parachute_radius, | |
| self.rocket.radius, | |
| ) | |
| ) | |
| * 3, | |
| (2 / 3) | |
| * math.pi | |
| * (min(self.parachute_radius, self.rocket.radius) ** 2) | |
| * self.parachute_height, |
Copilot
AI
Dec 14, 2025
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 code block is duplicated at lines 768-782. Consider extracting the parachute initialization callbacks into a helper method to avoid duplication and ensure both locations remain consistent. The duplication increases maintenance burden and the risk of inconsistencies if one location is updated but not the other.
Copilot
AI
Dec 14, 2025
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.
Attempting to delete the 't0' attribute before it has been set will raise an AttributeError. The 't0' attribute is only created later in u_dot_parachute (line 2774) using getattr with a default value. This callback should either be removed or moved to execute after the parachute phase ends, not when it begins.
| lambda self: delattr(self, "t0"), | |
| lambda self: delattr(self, "t0") if hasattr(self, "t0") else None, |
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.
Wha is the benefit behind this radius = self.parachute_radius?
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.
I thought it would help with the readability of the code, especially when the parameters are used in the convoluted equations. I can remove it
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.
I understand it.
it does not contribute so much to the readability actually.
We can save a few 0.0001 ns by removing this line
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.
calculating inflated_radius in the Flight class? Do you really need it?
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.
inflated_radius is the parachute radius in the current Flight stage, it is used to calculate the surface area of the parachute
Copilot
AI
Dec 14, 2025
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 surface area calculation can fail with a domain error when the eccentricity formula yields a negative value under sqrt. This can occur if inflated_height > inflated_radius when radius > height (line 2751), or if inflated_radius > inflated_height when radius <= height (line 2756). Add validation to ensure the discriminant is non-negative before taking the square root, or clamp the eccentricity to valid ranges [0, 1).
| e = math.sqrt(1 - (inflated_height**2) / (inflated_radius**2)) | |
| surface_area = ( | |
| math.pi * inflated_radius**2 * (1 + (1 - e**2) / e * math.atanh(e)) | |
| ) | |
| else: | |
| e = math.sqrt(1 - (inflated_radius**2) / (inflated_height**2)) | |
| surface_area = ( | |
| math.pi | |
| * inflated_radius**2 | |
| * (1 + inflated_height / (e * inflated_radius) * math.asin(e)) | |
| discriminant = 1 - (inflated_height**2) / (inflated_radius**2) | |
| if discriminant < 0: | |
| warnings.warn( | |
| f"Parachute eccentricity discriminant negative ({discriminant:.6f}); " | |
| "clamping to zero. Check parachute geometry parameters." | |
| ) | |
| discriminant = 0.0 | |
| elif discriminant > 1: | |
| warnings.warn( | |
| f"Parachute eccentricity discriminant > 1 ({discriminant:.6f}); " | |
| "clamping to one. Check parachute geometry parameters." | |
| ) | |
| discriminant = 1.0 | |
| e = math.sqrt(discriminant) | |
| surface_area = ( | |
| math.pi * inflated_radius**2 * (1 + (1 - e**2) / e * math.atanh(e)) if e != 0 else | |
| math.pi * inflated_radius**2 * 2 # limit as e->0 | |
| ) | |
| else: | |
| discriminant = 1 - (inflated_radius**2) / (inflated_height**2) | |
| if discriminant < 0: | |
| warnings.warn( | |
| f"Parachute eccentricity discriminant negative ({discriminant:.6f}); " | |
| "clamping to zero. Check parachute geometry parameters." | |
| ) | |
| discriminant = 0.0 | |
| elif discriminant > 1: | |
| warnings.warn( | |
| f"Parachute eccentricity discriminant > 1 ({discriminant:.6f}); " | |
| "clamping to one. Check parachute geometry parameters." | |
| ) | |
| discriminant = 1.0 | |
| e = math.sqrt(discriminant) | |
| surface_area = ( | |
| math.pi | |
| * inflated_radius**2 | |
| * (1 + inflated_height / (e * inflated_radius) * math.asin(e)) if e != 0 else | |
| math.pi * inflated_radius**2 * 2 # limit as e->0 |
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.
Since the inflated radius / inflated height = radius / height, the if else statement should already account for sign of the value under the square root
Copilot
AI
Dec 14, 2025
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 volume flow calculation assumes the parachute is vertical by using only freestream_z. However, parachutes don't necessarily descend perfectly vertically, especially in windy conditions or during initial deployment. The horizontal velocity components (freestream_x, freestream_y) should also contribute to the flow into the parachute. Consider using the total freestream velocity magnitude or projecting the velocity onto the parachute's orientation axis.
| # Calculate volume flow of air into parachute | |
| volume_flow = ( | |
| rho | |
| * freestream_z # considering parachute as vertical | |
| # Calculate volume flow of air into parachute using total freestream velocity magnitude | |
| freestream_velocity_magnitude = math.sqrt( | |
| freestream_x**2 + freestream_y**2 + freestream_z**2 | |
| ) | |
| volume_flow = ( | |
| rho | |
| * freestream_velocity_magnitude |
Copilot
AI
Dec 14, 2025
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 volume_flow calculation has incorrect units and logic. Currently, it computes rho * velocity * area, which yields mass flow rate (kg/s), not volume flow rate (m³/s). The variable should be renamed to 'mass_flow' and the integration on line 2779 should add mass, not volume. Alternatively, remove the rho multiplication here to get true volume flow, then multiply by rho only when computing the added mass on line 2782.
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.
do you really need to store step.t0? Why?
Should this really be a public attribute?
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.
I used it to have access to the time step between u_dot_parachute calls (not ideal). I can change it for a private attribute, or do you have any other suggestion?
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.
private is better
Copilot
AI
Dec 14, 2025
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 manual integration approach using timestep differences is inefficient and can introduce numerical errors. The ODE solver will call this derivative function multiple times per timestep with different intermediate values of t, but the integration assumes each call represents a true timestep. This will cause parachute_volume to accumulate incorrectly. Consider reformulating this as a proper differential equation by adding parachute_volume to the state vector u, and returning its time derivative as part of u_dot.
Copilot
AI
Dec 14, 2025
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 new parachute inflation model (lines 2741-2785) lacks test coverage. This is a significant new feature that modifies how parachute dynamics are simulated. Consider adding tests that verify: (1) parachute_volume increases over time during inflation, (2) the volume converges to the expected inflated volume, (3) the surface area calculations produce physically reasonable values, and (4) edge cases like very small or very large parachutes are handled correctly.
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.
@ArthurJWH why do you need to calculate the
parachute_volumewithin the flight simulation? Can you calculate and store it in the Parachute class?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.
My original idea was to assume the initial radius being the rocket radius (compartment from where it is ejected), but I cannot access the rocket radius within Parachute class. I can add a new parameter "initial radius" for the Parachute class, if you think that is better