Skip to content

ENH: Individual Fins #818

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

Open
wants to merge 36 commits into
base: develop
Choose a base branch
from
Open

ENH: Individual Fins #818

wants to merge 36 commits into from

Conversation

MateusStano
Copy link
Member

Checklist

  • Tests for the changes have been added (if needed)

Description

Individual Fins model have been added:

  • Usage can be seen in the documentation files. All equations and modelling are also described in the docs file.

  • A lot of structural changes were made to all fin classes. Please review this new structure with care.

  • If you run a simulation with the standard Fins model and another with individual fins, the results will be a little different. This happens because the individual fin model calculates forces for each fin separately, using the airflow at each fin’s exact position. The standard model (Barrowman) treats all the fins as one and uses a single point along the rocket’s center. In theory, this means that individual fins are more accurate then the standard model

  • There are a couple of TODOs relating to cant angle control. The center of pressure position relative to CDM is precalculated in the Rocket class and then accessed in the Flight. This causes a problem if the fin’s cant angle changes during the simulation because that changes the center of pressure position, but right now there’s no easy way to update these values in the Rocket while the simulation is running.

Breaking change

  • Yes
  • No

MateusStano and others added 30 commits September 7, 2024 22:52
Co-authored-by: kevin-alcaniz <[email protected]>
commit c674725
Author: Kevin Alcañiz <[email protected]>
Date:   Sat Apr 12 13:40:25 2025 +0200

    ENH: Introduce Net Thrust with pressure corrections (#789)

    * wind factor bug corrected

    the wind factor wasn't applied to the env.wind_velocity properties

    * BUG: StochasticModel visualize attributes of a uniform distribution

    It showed the nominal and the standard deviation values and it doesn't make sense in a uniform distribution. In a np.random.uniform the 'nominal value' is the lower bound of the distribution, and the 'standard deviation' value is the upper bound. Now, a new condition has been added for the uniform distributions where the mean and semi range are calculated and showed. This way the visualize_attribute function will show the whole range where the random values are uniformly taken in

    * variable names corrections

    * Corrections requested by the pylint test

    * ENH: Add pressure corrections for thrust in SolidMotor

    The thrust generated by a SolidMotor is now adjusted for the atmospheric pressure. To achieve that, a new attribute, 'vacuum_thrust', has been created. The 'net_thrust' is the result of 'vacuum_thrust' minus the atmospheric pressure multiplied by the nozzle area.

    * ENH: pylint recommendations done

    * ENH: net thrust method extended to the rest of the motor classes

    * BUG: __post_processed_variables inconsistent array

    * ENH: ruff reformatting

    * Update rocketpy/motors/motor.py

    Co-authored-by: Gui-FernandesBR <[email protected]>

    * ENH: Avoid breaking change

    * ENH: Pressure Thrust method added

    * BUG: call to the thrust function wrong

    * BUG: pressure thrust evaluated when motor is turned off

    * ENH: CHANGELOG updated

    * DOC: definition of exhaust velocity improved

    ---------

    Co-authored-by: Gui-FernandesBR <[email protected]>

commit 9f2644a
Author: Lucas Prates <[email protected]>
Date:   Sat Apr 12 11:27:53 2025 +0200

    ENH: Implement Multivariate Rejection Sampling (MRS) (#738)

    * ENH: implementing a draft version of the Multivarite Rejectio Sampler (MRS).

    * MNT: quick notebook to test MRS during development

    * MNT: refactoring class to match review suggestions

    * ENH: add comparison prints, plots and ellipses to MonteCarlo and finally checks in MRS

    * MNT: add MultivariateRejectionSampler class to inits and apply format

    * DOC: writting .rst documentation for MRS

    * MNT: adding pylint flags to skip checks

    * DOC: completing missing sections in mrs.rst

    * DOC: add changelog and apply sugestions in MRS class

    * DOC: apply suggestions to the MRS.rst

    * MNT: use Union instead of | for type hinting since we have to support python3.9

    * TST: adding unit and integration tests to MRS

    * MNT: use pylint flag to fix linter

    * TST: adding tests to MonteCarlo comparison features

    * MNT: applying suggestions in .rst, better handling nested variables in MRS and applying linters

    * MNT: removing TODO comments from monte_carlo_plots

    * MNT: remove useless TODO

    * MNT: inserting pragmas for no cover and resolving changelog conflict

commit d49c40e
Author: ArthurJWH <[email protected]>
Date:   Fri Apr 11 16:11:20 2025 -0400

    ENH: Create a rocketpy file to store flight simulations (#800)

    * ENH: added .rpy file functionality (see issue 668)

    This commit add 'save_to_rpy' and 'load_from_rpy' functions, that allows saving and loading flights.

    * MNT: adjusting minor changes to .rpy functions and tests.

    Formatted docstrings correctly.
    Reverted duplication of `test_encoding.py` files.
    Version warning will be called when loaded version is more recent.

    * MNT: incorporating previous comments

    Change file management from os to Path
    Adjust docstrings

    * DOC: Added comment about outputs in `to_dict` method

    * MNT: Refactoring `RocketPyDecoder` unpacking operation and other small adjustments

    * DOC: update changelog

    * STY: formatted according to ruff

    * MNT: changing `str | Path` operation to support Python 3.9

    * MNT: fixed trailing commas on .rpy and added shield against `ruff` formatting .rpy and .json files

    * MNT: fixing error related to `test_flight_save_load_no_resimulate`

    When `include_outputs` were set to `True`, it would try to include the additional data into the flight, breaking the test

    * MNT: fixing a typo and adding comment on test coverage

    ---------

    Co-authored-by: Gui-FernandesBR <[email protected]>

commit 6bf70f3
Author: Júlio Machado <[email protected]>
Date:   Sat Apr 5 15:08:53 2025 -0300

    ENH: Support for the RSE file format has been added to the library (#798)

    * ENH: Support for the RSE file format has been added to the library. The import_rse method in the Abstract Motor class and the load_from_rse_file method in the GenericMotor class are now available. With this update, the library natively supports Rock Sim software data, eliminating the need for users to manually convert motor files. The implementation was based on the import_eng and load_from_eng_file methods, utilizing Python's standard XML library.

    * ENH: Adding tests to the methods of .rse file treatment.

    * ENH: fixing mistakes on the method and test file

    * MNT: Running ruff

    * MNT: Adding the PR to CHANGELOG.md

commit 220bb59
Merge: 4a41f7a 4df0b38
Author: Gui-FernandesBR <[email protected]>
Date:   Thu Mar 27 06:14:22 2025 -0300

    Merge pull request #797 from RocketPy-Team/master

    Updates develop after 1.9.0

commit 4df0b38
Author: MateusStano <[email protected]>
Date:   Mon Mar 24 17:35:03 2025 +0100

    REL: Update version to 1.9.0 (#795)

commit 5328d66
Author: MateusStano <[email protected]>
Date:   Mon Mar 24 13:07:52 2025 +0100

    DEP: Remove Pending Deprecations and Add Warnings Where Needed (#794)

    * DEP: Add deprecation warnings for outdated methods and functions

    * DEP: Remove deprecated methods for NOAA RUC soundings and power drag plots

    * DEV: changelog

    * MNT: ruff

    * DEP: Update deprecation warning for post_process method to specify removal in v1.10

    * MNT: Remove unused imports

commit 76fb5ef
Merge: a4b42c3 4a41f7a
Author: Gui-FernandesBR <[email protected]>
Date:   Sun Mar 23 19:17:16 2025 -0300

    Merge pull request #793 from RocketPy-Team/develop

    DEV: Master to v1.9.0

commit 4a41f7a
Author: Kevin Alcañiz <[email protected]>
Date:   Sun Mar 23 21:52:51 2025 +0100

    ENH: Introduce the StochasticAirBrakes class (#785)

    * wind factor bug corrected

    the wind factor wasn't applied to the env.wind_velocity properties

    * BUG: StochasticModel visualize attributes of a uniform distribution

    It showed the nominal and the standard deviation values and it doesn't make sense in a uniform distribution. In a np.random.uniform the 'nominal value' is the lower bound of the distribution, and the 'standard deviation' value is the upper bound. Now, a new condition has been added for the uniform distributions where the mean and semi range are calculated and showed. This way the visualize_attribute function will show the whole range where the random values are uniformly taken in

    * variable names corrections

    * Corrections requested by the pylint test

    * ENH: add multiplication for 2D functions in rocketpy.function

    Added the ability to multiply functions with 2D domains in the __mul__ function

    * ENH: StochasticAirBrakes class created

    The StochasticAirBrakes class has been created. The __init__.py files in the stochastic and rocketpy folders have also been modified accordingly to incorporate this new class

    * ENH: set_air_brakes function created

    This functions appends an airbrake and controller objects previuosly created to the rocket

    * ENH: add StochasticAirBrake to rocketpy.stochastic_rocket

    Some functions has been modified and other has been created in order to include the new StochasticAirBrakes feature into the StochasticRocket class. A new function named 'add_air_brakes' has been created to append a StochasticAirBrakes and Controller objects to the StochasticRocket object. A new function '_create_air_brake' has been introduced to create a sample of an AirBrake object through a StochasticAirBrake object. Enventually, the 'create_object' function has been modified to add the sampled AirBrakes to the sampled Rocket

    * BUG: StochasticAirBrake object input in _Controller

    When defining the _Controller object a StochasticAirBrake was input. This is already corrected and a AirBrake object is now introduced

    * ENH: add time_overshoot option to rocketpy.stochastic_flight

    Since the new StochasticAirBrake class is defined, we need the 'time_overshoot' option in the Flight class to ensure that the time step defined in the simulation is the controller sampling rate. The MonteCarlo class has had to be modified as well to include this option.

    * DOC: StochasticAirBrakes related documentation added

    Documentation related to the StochasticAirBrakes implementation has been added in StochasticAirBrakes, StochasticRocket and Rocket classes.

    * ENH: pylint recommendations done

    * ENH: Reformatted files to pass Ruff linting checks

    * ENH: Update rocketpy/stochastic/stochastic_rocket.py

    Unnecessary comment

    Co-authored-by: Gui-FernandesBR <[email protected]>

    * DOC: improve drag curve factor definition in StochasticAirBrakes

    * ENH: Change assert statement to if

    Co-authored-by: Gui-FernandesBR <[email protected]>

    * DOC: better explanation of __mul__ function

    Co-authored-by: MateusStano <[email protected]>

    * ENH: delete set_air_brakes function for simplicity

    * DOC: CHANGELOG file updated

    ---------

    Co-authored-by: Gui-FernandesBR <[email protected]>
    Co-authored-by: MateusStano <[email protected]>

commit 90553f5
Author: Kevin Alcañiz <[email protected]>
Date:   Sun Mar 23 20:31:50 2025 +0100

    ENH: Add Eccentricity to Stochastic Simulations (#792)

    * wind factor bug corrected

    the wind factor wasn't applied to the env.wind_velocity properties

    * BUG: StochasticModel visualize attributes of a uniform distribution

    It showed the nominal and the standard deviation values and it doesn't make sense in a uniform distribution. In a np.random.uniform the 'nominal value' is the lower bound of the distribution, and the 'standard deviation' value is the upper bound. Now, a new condition has been added for the uniform distributions where the mean and semi range are calculated and showed. This way the visualize_attribute function will show the whole range where the random values are uniformly taken in

    * variable names corrections

    * Corrections requested by the pylint test

    * ENH: more intuitive uniform distribution display in StochasticModel

    Co-authored-by: MateusStano <[email protected]>

    * ENH: Eccentricities added to the StochasticRocket class

    A bug has been corrected in Flight class and an enhancement has been performed in the Rocket class as well

    * BUG: thrust eccentricity bug corrected

    eccentricity_y was defined by x coordinate and eccentricity_x was defined by y coordinate

    * BUG: Undo some Rocket class changes

    * ENH: add eccentricities to StochasticRocket

    * BUG: fix MonteCarlo eccentricity inputs

    * ENH: pylint and ruff recommended changes

    * TST: fix tests with eccentricity

    ---------

    Co-authored-by: Gui-FernandesBR <[email protected]>

commit 7348053
Author: Kevin Alcañiz <[email protected]>
Date:   Sun Mar 23 13:49:35 2025 +0100

    BUG: fix the wind velocity factors usage and better visualization of uniform distributions in Stochastic Classes (#783)

    * wind factor bug corrected

    the wind factor wasn't applied to the env.wind_velocity properties

    * BUG: StochasticModel visualize attributes of a uniform distribution

    It showed the nominal and the standard deviation values and it doesn't make sense in a uniform distribution. In a np.random.uniform the 'nominal value' is the lower bound of the distribution, and the 'standard deviation' value is the upper bound. Now, a new condition has been added for the uniform distributions where the mean and semi range are calculated and showed. This way the visualize_attribute function will show the whole range where the random values are uniformly taken in

    * variable names corrections

    * Corrections requested by the pylint test

    * ENH: more intuitive uniform distribution display in StochasticModel

    Co-authored-by: MateusStano <[email protected]>

    ---------

    Co-authored-by: MateusStano <[email protected]>

commit d2f89ba
Author: Leonardo Rosa <[email protected]>
Date:   Fri Mar 21 18:57:49 2025 -0300

    DEV: add requirements-tests.txt on make install target (#791)

    * DEV: adds 'pip install -r requirements-tests.txt' recipe to 'make install' target on Makefile

    Co-authored-by: Gui-FernandesBR <[email protected]>

commit 91ac567
Author: Leonardo Rosa <[email protected]>
Date:   Fri Mar 21 18:53:53 2025 -0300

    BUG: fixes get_instance_attributes for Flight objects containing a Rocket object without rail buttons (#786)

    * DOC: fixed a typo in funcify_method() description

    * TST: created test for get_instante_attributes() with flight without rail buttons

    * BUG: fixed __calculate_rail_button_forces() by assigning a Function(0) to null_force instead of an empty array

    * DEV: updates CHANGELOG

commit 9407470
Author: Leonard <[email protected]>
Date:   Wed Mar 19 16:01:59 2025 +0100

    BUG: fixed AGL altitude in _FlightPrints.events_registered (#788)

    * BUG: fixed AGL altitude in _FlightPrints.events_registered

    * updeted CHANGELOG
@MateusStano MateusStano self-assigned this May 16, 2025
@MateusStano MateusStano requested a review from a team as a code owner May 16, 2025 19:42
@MateusStano MateusStano added Enhancement New feature or request, including adjustments in current codes Aerodynamics Any problem to be worked on top of RocketPy's Aerodynamic labels May 16, 2025
@MateusStano MateusStano requested a review from Copilot May 16, 2025 19:42
Copy link

@Copilot 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 introduces an Individual Fins model to improve the accuracy of fin force calculations by computing forces per fin rather than using a single aggregated point. Key changes include:

  • Adding new fin classes and updating fin imports in multiple modules.
  • Enhancements for printing and plotting fin data, including new fin print and plot classes.
  • Updates to documentation reflecting the new individual fin model.

Reviewed Changes

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

Show a summary per file
File Description
rocketpy/rocket/aero_surface/fins/init.py Added new fin exports (Fin, EllipticalFin, FreeFormFin, and TrapezoidalFin)
rocketpy/rocket/aero_surface/aero_surface.py Introduced a rotation matrix for surface-to-body conversions and minor cleanup in force computations
rocketpy/rocket/init.py Updated imports to include new fin classes
rocketpy/prints/aero_surface_prints.py Added new printing methods/classes for individual fin types
rocketpy/plots/rocket_plots.py Updated aerodynamic surface validation and added code for drawing individual fin geometry
rocketpy/plots/aero_surface_plots.py Added multiple new plotting classes and methods to visualize individual fin shapes and parameters
Documentation files Updated indices and references to include the new individual fin model
Comments suppressed due to low confidence (1)

rocketpy/plots/rocket_plots.py:361

  • [nitpick] Consider vectorizing the fin rotation operation rather than iterating over points to improve performance.
for i, p in enumerate(points): points[i] = surface._rotation_fin_to_body @ Vector(p)

Copy link

codecov bot commented May 17, 2025

Codecov Report

Attention: Patch coverage is 61.25166% with 291 lines in your changes missing coverage. Please review.

Project coverage is 78.53%. Comparing base (4df0b38) to head (45cebc4).
Report is 22 commits behind head on develop.

Files with missing lines Patch % Lines
rocketpy/plots/aero_surface_plots.py 29.47% 67 Missing ⚠️
rocketpy/rocket/aero_surface/fins/fin.py 31.25% 66 Missing ⚠️
rocketpy/prints/aero_surface_prints.py 33.33% 28 Missing ⚠️
rocketpy/rocket/aero_surface/fins/_base_fin.py 73.07% 28 Missing ⚠️
rocketpy/plots/rocket_plots.py 38.63% 27 Missing ⚠️
...ocketpy/rocket/aero_surface/fins/elliptical_fin.py 37.03% 17 Missing ⚠️
rocketpy/rocket/aero_surface/fins/free_form_fin.py 39.13% 14 Missing ⚠️
...ketpy/rocket/aero_surface/fins/_free_form_mixin.py 90.08% 12 Missing ⚠️
...etpy/rocket/aero_surface/fins/_elliptical_mixin.py 73.80% 11 Missing ⚠️
...cketpy/rocket/aero_surface/fins/trapezoidal_fin.py 45.00% 11 Missing ⚠️
... and 2 more
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #818      +/-   ##
===========================================
- Coverage    79.11%   78.53%   -0.59%     
===========================================
  Files           96      106      +10     
  Lines        11575    12445     +870     
===========================================
+ Hits          9158     9774     +616     
- Misses        2417     2671     +254     

☔ 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.

Comment on lines +179 to +182
try:
print(f"Tip chord: {self.aero_surface.tip_chord:.3f} m")
except AttributeError:
pass # it isn't a trapezoidal fin, just don't worry about tip chord
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
try:
print(f"Tip chord: {self.aero_surface.tip_chord:.3f} m")
except AttributeError:
pass # it isn't a trapezoidal fin, just don't worry about tip chord
if isinstance(self.aero_surface, What you want):
print(f"Tip chord: {self.aero_surface.tip_chord:.3f} m")

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with the suggestion. hasattr could also work.

sweep_length=None,
sweep_angle=None,
airfoil=None,
name="Fins",
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
name="Fins",
name="Trapezoidal Fin",

self.plots = _TrapezoidalFinPlots(self)

def evaluate_center_of_pressure(self):
"""Calculates and returns the center of pressure of the fin set in local
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
"""Calculates and returns the center of pressure of the fin set in local
"""Calculates and returns the center of pressure of the fin in local

The tuple's second item is the unit of the angle of attack,
accepting either "radians" or "degrees".
name : str
Name of fin set.
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
Name of fin set.
The trapezoidal fin's name.

TrapezoidalFin.sweep_angle : float
Fins sweep angle with respect to the rocket centerline. Must
be given in degrees.
TrapezoidalFin.d : float
Copy link
Member

Choose a reason for hiding this comment

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

TrapezoidalFin.d

vs

TrapezoidalFin.diameter

TrapezoidalFins.fin_area : float
Area of the longitudinal section of each fin in the set.
TrapezoidalFins.f_ar : float
Aspect ratio of each fin in the set
Copy link
Member

Choose a reason for hiding this comment

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

We have only one fin in the set, no?

Suggested change
Aspect ratio of each fin in the set
Aspect ratio of each fin in the set

self.evaluate_center_of_pressure()
self.evaluate_lift_coefficient()
self.evaluate_roll_parameters()

def evaluate_center_of_pressure(self):
Copy link
Member

Choose a reason for hiding this comment

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

isnt the evaluate_center_of_pressure common for both TrapFin and TrapFins?


There are currently three ways to model the aerodynamic effects of fins in RocketPy:

1. By use of the :class:`rocketpy.GenericSurface` or :class:`rocketpy.LinearGenericSurface` classes,
Copy link
Member

Choose a reason for hiding this comment

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

This way you skip the "skip line" entry

Suggested change
1. By use of the :class:`rocketpy.GenericSurface` or :class:`rocketpy.LinearGenericSurface` classes,
1. By use of the :class:`rocketpy.GenericSurface` or :class:`rocketpy.LinearGenericSurface` classes, \

Comment on lines +29 to +32
return (
root_chord,
span,
)
Copy link
Member

Choose a reason for hiding this comment

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

do you really need the return?

Comment on lines +1009 to +1010
# TODO: this depends on cant angle, so it should somehow be
# recalculated whenever the cant angle of the fin changes
Copy link
Member

Choose a reason for hiding this comment

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

maybe what you want to do is to create a setter method to the cant_angle parameter?

@@ -1018,14 +1033,20 @@ def add_surfaces(self, surfaces, positions):
surfaces : list, AeroSurface, NoseCone, TrapezoidalFins, EllipticalFins, Tail, RailButtons
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
surfaces : list, AeroSurface, NoseCone, TrapezoidalFins, EllipticalFins, Tail, RailButtons
surfaces : list[AeroSurface], AeroSurface

Comment on lines +47 to +56
# Check if sweep angle or sweep length is given
if sweep_length is not None and sweep_angle is not None:
raise ValueError("Cannot use sweep_length and sweep_angle together")
elif sweep_angle is not None:
sweep_length = np.tan(sweep_angle * np.pi / 180) * span
elif sweep_length is None:
sweep_length = root_chord - tip_chord
else: # Sweep length is given
pass

Copy link
Member

Choose a reason for hiding this comment

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

(minor) these checks could be moved to an another method

@@ -423,7 +308,7 @@ def compute_forces_and_moments(
* omega[2]
/ 2
)
M3 = M3_forcing - M3_damping
M3 = M3_forcing + M3_damping
return R1, R2, R3, M1, M2, M3

def to_dict(self, include_outputs=False):
Copy link
Member

Choose a reason for hiding this comment

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

As both Fins and TrapezoidalFinsMixin have the to_dict method, which method will the TrapezoidalFins class use?

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, one could define the to_dict here as an abstractmethod, so the children classes must define the method.

Comment on lines +112 to +130
chord_length[i] -= x
else:
chord_length[i] += x

# Replace infinities and handle invalid values in chord_lead and chord_trail
for i in range(points_per_line):
if (
np.isinf(chord_lead[i])
or np.isinf(chord_trail[i])
or np.isnan(chord_lead[i])
or np.isnan(chord_trail[i])
):
chord_lead[i] = 0
chord_trail[i] = 0
if chord_length[i] < 0 or np.isnan(chord_length[i]):
chord_length[i] = 0
if chord_length[i] > chord_trail[i] - chord_lead[i]:
chord_length[i] = chord_trail[i] - chord_lead[i]

Copy link
Member

Choose a reason for hiding this comment

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

Should be faster (copilot suggestion), and by far more readable:

        # Replace infinities and handle invalid values in chord_lead and chord_trail
        mask_invalid = (
            np.isinf(chord_lead) | np.isinf(chord_trail) | np.isnan(chord_lead) | np.isnan(chord_trail)
        )
        chord_lead[mask_invalid] = 0
        chord_trail[mask_invalid] = 0

        chord_length = np.where((chord_length < 0) | np.isnan(chord_length), 0, chord_length)
        max_chord = chord_trail - chord_lead
        chord_length = np.minimum(chord_length, max_chord)

Comment on lines +38 to +39
fin shapes."""
# pylint: disable=invalid-name
Copy link
Member

Choose a reason for hiding this comment

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

The evaluate_geometrical_parameters method is too long.
If possible, please consider refactoring it to split the same code into smaller pieces.

Comment on lines +14 to +24
down = False
for i in range(1, len(shape_points)):
if shape_points[i][1] > shape_points[i - 1][1] and down:
warnings.warn(
"Jagged fin shape detected. This may cause small inaccuracies "
"center of pressure and pitch moment calculations."
)
break
if shape_points[i][1] < shape_points[i - 1][1]:
down = True
i += 1
Copy link
Member

Choose a reason for hiding this comment

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

separate method

None
"""
# Center of pressure position in local coordinates
cpz = 0.288 * self.root_chord
Copy link
Member

Choose a reason for hiding this comment

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

0.288?

Where does it come from?
Any reference?

Comment on lines +184 to +199
def to_dict(self, include_outputs=False):
data = super().to_dict(include_outputs)
if include_outputs:
data.update(
{
"Af": self.Af,
"AR": self.AR,
"gamma_c": self.gamma_c,
"Yma": self.Yma,
"roll_geometrical_constant": self.roll_geometrical_constant,
"tau": self.tau,
"lift_interference_factor": self.lift_interference_factor,
"roll_damping_interference_factor": self.roll_damping_interference_factor,
"roll_forcing_interference_factor": self.roll_forcing_interference_factor,
}
)
Copy link
Member

Choose a reason for hiding this comment

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

nice!

But who is super here? the mixin or the fin?

self.evaluate_lift_coefficient()
self.evaluate_roll_parameters()

def evaluate_single_fin_lift_coefficient(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

A docstring would help here. This method is useful for both Fin and the current Fins classes, right? Otherwise, it should be only in the child.

)

# Differentiating at alpha = 0 to get cl_alpha
clalpha2D_incompressible = self.airfoil_cl.differentiate_complex_step(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why using complex_step here, is performance that crucial? I think it is a bit less stable in the Function class, since it was mainly tested only for lambda Functions. I have no idea if it works properly in the case of spline or polynomial interpolations.

Comment on lines +179 to +182
try:
print(f"Tip chord: {self.aero_surface.tip_chord:.3f} m")
except AttributeError:
pass # it isn't a trapezoidal fin, just don't worry about tip chord
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with the suggestion. hasattr could also work.

@@ -179,14 +274,26 @@ class _TrapezoidalFinsPrints(_FinsPrints):
"""Class that contains all trapezoidal fins prints."""


class _TrapezoidalFinPrints(_FinPrints):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need these default child classes? I understand their value for scalability (we can add things to them if needed), but couldn't we use the base _FinPrints?

import numpy as np


class _EllipticalMixin:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand the use of mixins so that you provide custom geometrical methods for each fin type. My question is why not simply use a bit of composition here (e.g. more like a strategy pattern): have a geometry attribute that handles all the specifics of geometry (a class _FinGeometry that has essentially the same methods as the mixin).

My question arises, since using mixins here is alright from a functional standpoint (it works), but it is a bit unconventional given that it is more used to provide more I/O related methods or "secondary" behavior to a class in general.

My brainstorming is something like:

class _BaseFin(ABC):
     def __init__(self, geometry, any common non geometric attributes (if any)):
        self.geometry = geometry
        ...
    # Ideally most of the code here should be concrete
    # and delegate things to the geometry (e.g. `return self.geometry.lift_coefficients`)

class TrapezoidalFins(_BaseFin):
     def __init__(self, same init as today):
         geometry = _TrapezoidalSetGeometry(geometry args)
         super().__init__(geometry)

class _FinGeometry(ABC):
    # Basically all abstractmethods to define structure

class _FinSetGeometry(_FinGeometry):
    # Any common specific logic of finsets
    # if no big specific logic, this class is not needed

class _TrapezoidalSetGeometry(_FinSetGeometry):
   ...

class _IndividualFinGeometry(_FinGeometry):
    # Any common specific logic of finsets (e.g. frame rotations)

class _TrapezoidalFinGeometry(_IndividualFinGeometry):
   ...

This is simply a brainstorm of mine I just had while reading the code. I believe this captures semantically a bit more the structure than mixins, while keeping flexibility.

On the many setters that update the _BaseFin geometry (e.g. change on span), I would simply create a new geometry and set the self.geometry. If performance is a concert, then make setters inside the geometry that are called.

data["cant_angle"],
data["airfoil"],
data["name"],
n=data["n"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good change.

"""
self._components.sort(key=lambda x: x.position.z, reverse=reverse)
components = deepcopy(self)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this method should not mutate the instance?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Aerodynamics Any problem to be worked on top of RocketPy's Aerodynamic Enhancement New feature or request, including adjustments in current codes
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

4 participants