Skip to content

Conversation

MateusStano
Copy link
Member

@MateusStano MateusStano commented Feb 16, 2022

Continuing #117 that was accidentally closed

Pull request type

Please check the type of change your PR introduces:

  • Code base additions (bugfix, features)
  • Code maintenance (refactoring, formatting, renaming, tests)
  • ReadMe, Docs and GitHub maintenance
  • Other (please describe):

Pull request checklist

Please check if your PR fulfills the following requirements, depending on the type of PR:

  • Code base additions (for bug fixes / features):

    • Tests for the changes have been added
    • Docs have been reviewed and added / updated if needed
    • Lint (black rocketpy) has passed locally and any fixes were made
    • All tests (pytest --runslow) have passed locally

What is the current behavior?

The current calculation of the normal force coefficient derivative for airfoil fins assumes that the term where the derivative of the lift coefficient is multiplied by alpha is a constant value. However, for curves that are not linear that simplification does not apply due to another intermediate derivative term.

What is the new behavior?

The calculations for the lift coefficient now use a more generalized equation based on thin airfoil theory, which is more in the lines of what Barrowman showed in his paper. The differences in calculating the lift with and without airfoils is only considered in the Cnalpha0 (see equation 3-1 in Barrowman). For planar fins, Cnalpha0 is considered to be 2π / beta. For airfoil fins, Cnalpha0 comes from the derivation of a lift coefficient curve that must be given as an input to the addFins method.

Does this introduce a breaking change?

  • Yes
  • No

Other information

The addition of the beta parameter makes it so the calculations for fins lift coefficient take into account the variation of the Mach number, which in turn can change a few other behaviors in the simulation (i.e. the static margin). This is worth noting since the other aerodynamic surfaces (nosecone and tail) don't consider the Mach number (yet).

@MateusStano MateusStano self-assigned this Feb 16, 2022
@Gui-FernandesBR Gui-FernandesBR added the Enhancement New feature or request, including adjustments in current codes label Feb 16, 2022
Copy link
Member

@giovaniceotto giovaniceotto left a comment

Choose a reason for hiding this comment

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

Finally! Finally, a perfect airfoil implementation! Great work @MateusStano, we really needed this!

I am suggesting a few simple changes, see what you think about them!

Oh, one more thing, check your black! It is replacing all the **.

@Gui-FernandesBR
Copy link
Member

image

Am I doing something wrong or the getting_started_colab.ipynb is no longer working on this branch's Pull Request ?

@MateusStano
Copy link
Member Author

MateusStano commented Feb 18, 2022

image

Am I doing something wrong or the getting_started_colab.ipynb is no longer working on this branch's Pull Request ?

Uhh i think something is wrong. It is not running the new code. That error shows the old airfoil implementation

@Gui-FernandesBR
Copy link
Member

OK I fixed my code here.

In my opinion it's brillant, we can move forward. Congratulations @MateusStano really easy to understand the code.

Sugestion of creating good plots of airfoils data in the future, embedded on allInfo()

@Gui-FernandesBR Gui-FernandesBR added the Bug Something isn't working label Feb 18, 2022
@giovaniceotto giovaniceotto self-requested a review February 19, 2022 16:59
@giovaniceotto
Copy link
Member

giovaniceotto commented Feb 19, 2022

Sugestion of creating good plots of airfoils data in the future, embedded on allInfo()

Can we add this to our action log? @Gui-FernandesBR

@giovaniceotto
Copy link
Member

I'll finish my last review by tonight so that we can merge it!

Copy link
Member

@giovaniceotto giovaniceotto left a comment

Choose a reason for hiding this comment

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

Three more simple, but important, suggestions! If you accept them all, we can merge it. But check if you really agree with them.

Two of them are simple ones regarding documentation. However, the third one is a bit more complex and important.

MateusStano and others added 3 commits February 20, 2022 15:14
Co-authored-by: Giovani Hidalgo Ceotto <[email protected]>
Co-authored-by: Giovani Hidalgo Ceotto <[email protected]>
Co-authored-by: Giovani Hidalgo Ceotto <[email protected]>
@giovaniceotto
Copy link
Member

@Gui-FernandesBR, when you approve, feel free to merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Enhancement New feature or request, including adjustments in current codes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rocket.addFins() airfoil argument should support floats and lists Rocket.addFins() does not support airfoil lift coefficient data with Cl=0
3 participants