Skip to content

MAINT: move last plots from Environment class #326

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 3 commits into from
Feb 19, 2023

Conversation

Gui-FernandesBR
Copy link
Member

Pull request type

Please check the type of change your PR introduces:

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

Pull request checklist

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

  • 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?

The very same behavior, the very same results, but now there is no plt.plot(... inside the Enrivonment class

Does this introduce a breaking change?

  • No

Other information

This PR could be seen as an extension of #296

@github-actions github-actions bot requested a review from MateusStano January 25, 2023 05:59
@Gui-FernandesBR Gui-FernandesBR added this to the Release v1.0.0 milestone Jan 25, 2023
@Gui-FernandesBR Gui-FernandesBR added Refactor Outputs Dedicated to visualizations enhancements like prints and plots labels Jan 25, 2023
@Gui-FernandesBR
Copy link
Member Author

Ready for review ✅👌

Copy link
Member

@MateusStano MateusStano left a comment

Choose a reason for hiding this comment

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

Everything seems good! Just one thing:

image

The lines are above the legend. Maybe this can be fixed using the zorder param of matplotlib?

@Gui-FernandesBR
Copy link
Member Author

Everything seems good! Just one thing:

image

The lines are above the legend. Maybe this can be fixed using the zorder param of matplotlib?

Very good catch man! Yeah, zorder is a good option. Or maybe we could set the legend after setting the plot (however I think this is already happening).
What I think is weird is that only the speed of sound have label/lengend. Any reason to that?

@MateusStano
Copy link
Member

What I think is weird is that only the speed of sound have label/lengend. Any reason to that?

Yeah, that is weird. Couldn't figure out how to fix it but found two things:

  1. The name of the axes labels are wrong
  2. And more importantly, those plots don't need legend because the different axes are already displayed in different colors !!

I think just removing the legend here will work

@Gui-FernandesBR
Copy link
Member Author

Ready for a re-review @MateusStano

@Gui-FernandesBR Gui-FernandesBR changed the base branch from maint/extract-logic-prints to beta/v1.0.0 February 19, 2023 22:27
@Gui-FernandesBR Gui-FernandesBR merged commit 483d9cf into beta/v1.0.0 Feb 19, 2023
@Gui-FernandesBR Gui-FernandesBR deleted the maint/extract-logic-prints-env branch April 10, 2023 22:50
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.

3 participants