Skip to content

MAINT: move aero surface plots and prints #381

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

Merged
merged 8 commits into from
Jun 27, 2023

Conversation

Gui-FernandesBR
Copy link
Member

Pull request type

  • Code maintenance (refactoring, formatting, renaming, tests)

Pull request checklist

  • 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

What is the new behavior?

  • Plots and prints related to Aero
  • Huge effort of abstracting plots and prints classes, just like the AeroSurface class is currently structured.

Does this introduce a breaking change?

  • Yes (there's no longer a draw() method in the fins' classes)

Other information

  • Notebooks are running as they always were.
  • The tests weren't modified, so all the changes happened exclusively in the background. This should make the review much faster.
  • This will create several conflicts with the ENH: nosecone draw and bluffness  #372 PR, sorry.
  • Please check for plural/singular in the name of the new classes that were created.

@Gui-FernandesBR Gui-FernandesBR added this to the Release v1.0.0 milestone Jun 23, 2023
@github-actions github-actions bot requested a review from MateusStano June 23, 2023 05:20
@Gui-FernandesBR Gui-FernandesBR added Refactor Outputs Dedicated to visualizations enhancements like prints and plots labels Jun 23, 2023
Comment on lines 94 to 95
# Check if draw method is working properly
assert FinSet.draw() == None
Copy link
Member

Choose a reason for hiding this comment

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

Why remove this?

Copy link
Member Author

Choose a reason for hiding this comment

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

The Fin.draw() method doesn't exist anymore. Only the Fin.plots.draw()

Copy link
Member Author

Choose a reason for hiding this comment

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

If you really want to, we can create a draw() method inside each of the Fins classes, which will return self.plots.draw(), but I honestly don't see how that could be helpful.

@Gui-FernandesBR
Copy link
Member Author

Ready for a re-review @MateusStano

@MateusStano MateusStano merged commit 889abcf into beta/v1.0.0 Jun 27, 2023
@Gui-FernandesBR Gui-FernandesBR deleted the maint/move-aero-surface-plots branch June 27, 2023 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Outputs Dedicated to visualizations enhancements like prints and plots Refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants