-
-
Notifications
You must be signed in to change notification settings - Fork 167
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
ENH: Plotting both power on and off drag curves in a single plot. #547
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #547 +/- ##
===========================================
- Coverage 72.32% 72.31% -0.02%
===========================================
Files 56 56
Lines 9399 9416 +17
===========================================
+ Hits 6798 6809 +11
- Misses 2601 2607 +6 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent PR, I went through the PR description and everything was transparent enough.
I'm adding some commentaries with concerns that I have, it would be good to discuss them if needed. Also, please let me know if there's anything you don't understand at a first glance.
Welcome to the Matplotlib School of Engineering!
MNT: removing the unnecessary 'return None' line Co-authored-by: Gui-FernandesBR <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM,
Thank you for your first contribution.
It does solve the problem and make our lives better when seeing all those plots.
As a suggestion, mext PR I recommend you to also include a picture of the plot you make so we can have a preview before reviewing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! This is much better to compare the drag curves and adding the transonic region is the icing on the cake.
Pull request type
Checklist
black rocketpy/ tests/
) has passed locallypytest --runslow
) have passed locallyCHANGELOG.md
has been updated (if relevant)Current behavior
Before the changes, the drag curves were plotted separated.
New behavior
Now, with the changes made, there is only a plot with both curves.
Breaking change
Additional information
The methods power_on_drag and power_off_drag of the _RocketPlots class were removed.