ENH: Elliptical fins added to Rocket class #172
Conversation
…ter/RocketPy into enh/elliptical_fins
Class addFins updates: - Parameters: Now we have a new parameter "type", a string to set the type of the fins. - Span, Root and Tip parameters were removed and a dictionary was added. - Elliptical fins added
AddFins class optimized: - Repeated codes deleted - Roll geometry constant added to elliptical fins - Elliptical fins code finished
- Repeated codes deleted - Roll geometry constant added to elliptical fins - Elliptical fins code finished
KrWanderley
requested review from
Gui-FernandesBR and
giovaniceotto
and removed request for
giovaniceotto
|
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 |
…etrical Constant; testing
|
@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 |
Gui-FernandesBR
requested review from
giovaniceotto
and removed request for
giovaniceotto
|
@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 |
ENH: Refactor Rocket.addFins to avoid breaking changes
|
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)
|
There was a problem hiding this comment.
Great work, thank you all!
@MateusStano or @giovaniceotto feel free to merge once you review and double check
Pull request type
Please check the type of change your PR introduces:
Pull request checklist
Please check if your PR fulfills the following requirements, depending on the type of PR:
ReadMe, Docs and GitHub maintenance:
Code base maintenance (refactoring, formatting, renaming):
black rocketpy) has passed locally and any fixes were madepytest --runslow) have passed locallyCode base additions (for bug fixes / features):
black rocketpy) has passed locally and any fixes were madepytest --runslow) have passed locallyWhat 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?
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.