Skip to content
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

MNT: Fix env plots max heights #433

Merged
merged 8 commits into from
Oct 9, 2023

Conversation

Gui-FernandesBR
Copy link
Member

Pull request type

  • Code changes (bugfix, features)
  • Code maintenance (refactoring, formatting, tests)
  • ReadMe, Docs and GitHub updates
  • Other (please describe):

Checklist

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

Current behavior

  • Extremally annoying to see the gravity plot on the oposite orientation relative to the other plots in Environment.all_info()
  • Thanks to the following code line, the refreshment of the max_expected_height variable wasn't being updated after the self.plots initialization.
    self.grid = np.linspace(environment.elevation, environment.max_expected_height)

New behavior

  • There is a simple setter for the max_expected_height, which will also update the grid value in the plots.. I considered different approaches, and implemented the best that I found.
  • I removed some code repetitions from the Environment.init() method. The class initialization should be faster now.

Breaking change

  • Yes
  • No

Additional information

  • In the future, I would love to see the same plots with scatters over the lines, just to check the data retrieved from the APIs. Not a job for this PR tho.

@Gui-FernandesBR Gui-FernandesBR added Outputs Dedicated to visualizations enhancements like prints and plots Environment Enviroment Class related features labels Oct 9, 2023
@Gui-FernandesBR Gui-FernandesBR added this to the Release v1.X.0 milestone Oct 9, 2023
@Gui-FernandesBR Gui-FernandesBR self-assigned this Oct 9, 2023
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@Gui-FernandesBR Gui-FernandesBR changed the base branch from master to develop October 9, 2023 03:22
@MateusStano
Copy link
Member

Wow! Finally someone fixed this!

I would just suggest to add some env.max_expected_height parameters to notebooks/docs pages

@MateusStano
Copy link
Member

Also, just thought about this, but maybe max_expected_height should be an optional argument in Environment Init

@Gui-FernandesBR
Copy link
Member Author

Also, just thought about this, but maybe max_expected_height should be an optional argument in Environment Init

Solved! Although it is currently kinda useless, it has the potential for allowing us to speed up the Environment initialization

@Gui-FernandesBR
Copy link
Member Author

Wow! Finally someone fixed this!

I would just suggest to add some env.max_expected_height parameters to notebooks/docs pages

I updated the geting started notebook, I can update the other in another moment.

@Gui-FernandesBR Gui-FernandesBR merged commit 4206b85 into develop Oct 9, 2023
9 checks passed
@Gui-FernandesBR Gui-FernandesBR deleted the mnt/fix-env-plots-max-heights branch October 9, 2023 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Environment Enviroment Class related features Outputs Dedicated to visualizations enhancements like prints and plots
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants