RocketPy

RocketPy copied to clipboard

Pull request type

Please check the type of change your PR introduces:

• [x] 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:

• ReadMe, Docs and GitHub maintenance:

  • [ ] Spelling has been verified
  • [ ] Code docs are working correctly

• Code base maintenance (refactoring, formatting, renaming):

  • [ ] 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

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

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

What is the current behavior?

RocketPy only accept trapezoidal fins.

What is the new behavior?

Now, RocketPy accept elliptical fins. To do that, Barrowman equations and some deductions were used to define center of mass and lift coeficients. All the equations can be found on the PDF: https://github.com/Projeto-Jupiter/RocketPy/blob/a4778ba969f044ff669bc55cf35a572f2927b61f/docs/technical/aerodynamics/Elliptical_Fins.pdf

Does this introduce a breaking change?

• [x] Yes
• [ ] No

It's a breaking change because the way fins are added on addFins method was changed: before that, fins were defined by the 3 input parameters and now it's defined by a dictionary, which breakes older codes.

Other information

Due to the significant changes in the way fins are added in the addFins method, several tests will have to be updated. Furthermore, new tests for elliptical fins still must be created. Moreover, all the Getting Started notebooks must be updated.

As this is introducing a breaking change, we might adjust the instances on our example notebooks in order to ensure verything runs without problem after merging. Here is an example of what can be done:

Original code:

FinSet = Calisto.addFins(
    4, span=0.100, rootChord=0.120, tipChord=0.040, distanceToCM=-1.04956
)

After introducing some changes to accomplish with the PR:

FinSet = Calisto.addFins(type="trapezoid",
                         n=4,
                         finParameters={"span": 0.100, 
                                        "rootChord": 0.120,
                                        "tipChord": 0.040},
                         distanceToCM=-1.04956,
                         radius=0,
                         cantAngle=0,
                         airfoil=None,)

The files we need to change are the following:

• [x] docs\notebooks\getting_started.ipynb
• [x] docs\notebooks\getting_started_colab.ipynb
• [x] docs\notebooks\dispersion_analysis\dispersion_analysis.ipynb

Also, we should be worried about unit test! This can be overhelmed but extremally important to be set. Basically we need modify all test files that import and use rocket class, always trying to accomplish with the new version you implemented. Again, just let me know if you need any help.

Elliptical_Fins_revised_guilherme.pdf

Hey, I'm uploading a revised version of your pdf file, hope you can open the commentaries on any pdf viewer.

Now I believe my revision is done, I have to say that I really liked your implementation and I'm willing to see you guys addressing all the commentaries and therefore being able to merge.

We will have a new release by the end of this month, and it would be nice having the elliptical fins released as well. I'm available for anything u need, just let me know

@FranzYuri in case you have finished addressing all commentaries please request a re-review

Tks @FranzYuri for solving those issues!

I'll wait for @giovaniceotto review as well.

I'm trying to solve some conflicts!

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

@giovaniceotto please give us your review so we can define if we proceed modifying tests due to breaking changes or not.

@FranzYuri we still need couple minor changes on the pdf file

Off-topic: Solving conflicts by using vscode seems easier now! I hope this is a new thing, otherwise I was struggling a lot more than needed when tried to solve conflicts before (LoL)