ENH: Refactor Aero Surface, Make GenericSurface the Mother Class#1064
ENH: Refactor Aero Surface, Make GenericSurface the Mother Class#1064MateusStano wants to merge 12 commits into
GenericSurface the Mother Class#1064Conversation
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Every aerodynamic surface is now rooted in GenericSurface and stores its force coefficients in the body frame (cN/cY/cA) plus the cm/cn/cl moments, exposing all nine coefficients (cL/cD/cQ/cN/cY/cA/cm/cn/cl) with the wind trio lazily derived. A force_convention argument lets users supply wind- or body-frame coefficients. Barrowman surfaces (nose, tail, fin sets) keep the classic geometric normal-force/moment method, report the force at the geometric center of pressure via the classic 180-degree surface rotation, and expose cN_alpha/cY_beta stability slopes (the old clalpha relabelled). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the poorly defined calisto_linear_generic fixture, which kept the Barrowman nose cone and tail and swapped only the fins for a LinearGenericSurface with arbitrary made-up coefficients. The new fixture is a standalone Calisto whose nose cone, tail and fins are all LinearGenericSurfaces built from coefficient curves extracted off the standard Barrowman surfaces (normal-force-curve slope, center of pressure, fin roll damping). Each linear surface applies its force at its own origin and is placed at the source surface's center-of-pressure station, so both the static-margin path and the flight-moment path land at the same point as calisto_robust. The resulting flight matches the standard Calisto (identical apogee, out-of-rail time and ascent angle of attack), so the fixture now exercises the linear generic-surface path against a known-good reference. Also fix test_linear_generic_surface_flight_is_stable to check the angle of attack only during the ascent off the rail: on the rail the freestream speed is ~0 and the angle of attack is reported as a degenerate 90 degrees for any launcher, which previously failed the < 45 assertion. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## enh/events #1064 +/- ##
=============================================
Coverage ? 81.88%
=============================================
Files ? 129
Lines ? 16880
Branches ? 0
=============================================
Hits ? 13822
Misses ? 3058
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| The ``reynolds`` axis is by default based on the reference length (the | ||
| rocket diameter). Published rocket data often bases the Reynolds number on | ||
| the **body length** instead, which for a slender rocket is much larger. If | ||
| your table uses a different length, pass it as ``reynolds_length`` when | ||
| creating the surface so the Reynolds number the simulation feeds your table | ||
| matches the one it was built against. |
There was a problem hiding this comment.
This phrase is a bit odd, especially the ending "so the Reynolds number the simulation...".
What if the user table is based on the reference length and reynolds_length is given? Good docstrings here is important to avoid confusion.
| Activation Window | ||
| ----------------- | ||
|
|
||
| By default a surface produces aerodynamic force throughout the flight. The | ||
| ``active_during`` argument restricts it to part of the flight. This is useful | ||
| for a surface that only exists (or only matters) during a phase. It is accepted | ||
| by both :class:`rocketpy.GenericSurface` and | ||
| :class:`rocketpy.LinearGenericSurface`, and accepts: | ||
|
|
||
| - ``"always"`` (default): the surface always contributes force. | ||
| - ``"power_on"``: only while the motor is burning (up to burnout). | ||
| - ``"power_off"``: only after the motor has burned out. | ||
| - a callable ``active_during(t, flight)`` returning ``True`` when the surface is | ||
| active at time ``t`` (in seconds) of the given :class:`rocketpy.Flight`, for | ||
| any custom window. | ||
|
|
||
| .. code-block:: python | ||
|
|
||
| # base drag that only applies after burnout | ||
| base_drag = GenericSurface( | ||
| reference_area=rocket.area, | ||
| reference_length=2 * rocket.radius, | ||
| coefficients={"cA": 0.4}, # axial (drag) coefficient | ||
| active_during="power_off", | ||
| ) | ||
|
|
||
| This is also how a full-vehicle model captures the powered/coasting drag | ||
| difference: build one ``"power_on"`` and one ``"power_off"`` surface and add them | ||
| together (see :ref:`fullbodyaerodynamics`). |
There was a problem hiding this comment.
Wouldn't it be more fitting to accept (also) an Event here? Under the hood, are the default options power_on and power_off converted to a simulation Event?
| Because it is just another aerodynamic surface, a full-vehicle model can be | ||
| **mixed** with modelled add-on surfaces (e.g. a measured body plus modelled | ||
| canards). Pass ``overwrite=True`` to make it the rocket's **only** aerodynamics: | ||
| every existing aerodynamic surface is removed and both built-in drag curves are | ||
| cleared, so the supplied surface(s) provide the complete force set. | ||
|
|
There was a problem hiding this comment.
Maybe a small section or a few more words to clarify exactly when which drag is used could be beneficial. If I understood it correctly:
Rocket.power_on_dragandRocket.power_off_drag: are the default and are used whenGenericSurfacesare present (e.g. fins, tail, nosecones etc). The other aerodynamic parameters (e.g. lift) do come from a composition of theGenericSurfaces, i.e. only drag is ignored (except if theactive_duringis given, which adds to the rocket drag);Rocket.add_full_body_aerodynamicswithoverwrite=False: as a normalGenericSurface, the drag still is theRocket.power_on_dragandRocket.power_off_drag. It could be briefly clarified what is the mathematical/modelling difference of this surface to the non-full-body ones;Rocket.add_full_body_aerodynamicswithoverwrite=True: theRocket.power_on_dragandRocket.power_off_dragare ignored in favor of the surface.
Maybe I am mixing things up, but if I got it right, maybe renaming overwrite to overwrite_rocket_drag could help here.
| Extracting a rocket's coefficients | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| You can also collapses an assembled rocket into a single |
There was a problem hiding this comment.
| You can also collapses an assembled rocket into a single | |
| You can also collapse an assembled rocket into a single |
| - ``"always"`` (default): the surface always contributes force. | ||
| - ``"power_on"``: only while the motor is burning (up to burnout). | ||
| - ``"power_off"``: only after the motor has burned out. | ||
| - a callable ``active_during(t, flight)`` returning ``True`` when the surface is | ||
| active at time ``t`` (in seconds) of the given :class:`rocketpy.Flight`, for | ||
| any custom window. |
There was a problem hiding this comment.
I understand that this is rather intuitive for 99% of rockets and users. However I would like to bring to discussion whether the naming/division power_on and power_off is adequate model flexibility and future proofing. I ask this, since it has a few assumptions:
- Assumes a single engine rocket;
- Assumes the effect of the engine on drag is always the same, no matter the throttling or TVC.
Maybe there are more. Overall it feels a bit restrained in terms of design (I understand having this as default/pre-built/easy to use options), but having the option to tweak the parameters (not only toggle) based on the any flight parameter or on an Event might be interesting.
| Choosing an extrapolation method | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| Extrapolation controls the behavior *outside* the tabulated range. The options | ||
| are ``"constant"``, ``"natural"`` and ``"zero"``. This choice matters more than | ||
| interpolation, because a bad one fails silently, precisely when the rocket is at | ||
| an extreme condition beyond your data. | ||
|
|
||
| - ``"constant"`` holds the value at the nearest edge of the data. This is the | ||
| **default for tabulated coefficients**, and the right choice for essentially | ||
| all of them: a rocket can briefly exceed your tabulated Mach/angle range, and | ||
| holding the last value is bounded and physically conservative. | ||
| - ``"zero"`` returns 0 outside the range. Occasionally reasonable for force or | ||
| moment *slopes* if you want contributions to vanish past the modeled envelope, | ||
| but it introduces a discontinuity at the edge. | ||
| - ``"natural"`` continues the fitted curve past the data. **Avoid this for | ||
| tabulated coefficients**: extrapolating a linear or spline fit can send | ||
| :math:`C_A` or a moment slope to large, non-physical values right when the | ||
| rocket is at an extreme condition. | ||
|
|
||
| .. tip:: | ||
| Tabulated coefficients default to ``extrapolation="constant"`` so they never | ||
| run to non-physical values past the tabulated envelope. Override it only when | ||
| you have a specific reason (e.g. ``"zero"`` to make a contribution vanish | ||
| outside the modeled range). | ||
|
|
||
| .. seealso:: | ||
| These arguments are forwarded to each :class:`rocketpy.Function`; see | ||
| :meth:`rocketpy.Function.set_interpolation` and | ||
| :meth:`rocketpy.Function.set_extrapolation` for the full list of methods. |
There was a problem hiding this comment.
Overall, should the recommendation of "constant" extrapolation be valid for both LinearGenericSurface (based on derivatives) and GenericSurface? I ask this, since maybe it is a matter of keeping consistency: if a coefficient has a constant extrapolation, its derivative extrapolation should likely be zero.
I don't know what is more appropriate in this case, but in the Motor class, for instance, the Motor.propellant_mass has constant extrapolation, whilst the Motor.net_mass_flow_rate has zero extrapolation exactly for this reason. I don't know, maybe the most logical for this surfaces case would be constant extrapolation for derivative coefficients and (consequentially) linear extrapolation for the normal coefficients.
| extrapolation=extrapolation, | ||
| interpolation=interpolation, |
There was a problem hiding this comment.
I would probably make these two **kwargs or after a ,*,. I believe their use here is more justified, but the same parameter is present as positional in the Motor class and I feel it is very underused/not useful there.
| center_of_pressure=(self.cpx, self.cpy, self.cpz), | ||
| name=self.name, | ||
| ) | ||
| self._barrowman_initialized = True |
There was a problem hiding this comment.
The architecture here is a bit convoluted, I understand the reasoning, but it is hard to follow due to the amount of methods that need to be called at specific places in the code (e.g. before barrowman/post barrowman) and that have similar names (update_coefficient/calculate_all_coefficients etc).
Another factor _update_geometry_chain() sometimes constructs the superclass and sometimes updates it, which also involves a fair bit of care.
2 participants
TBD