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

BUG: env analysis altitudes ASL mistake (solve #371) #402

Merged
merged 2 commits into from
Sep 6, 2023

Conversation

Gui-FernandesBR
Copy link
Member

@Gui-FernandesBR Gui-FernandesBR commented Sep 3, 2023

Pull request type

Please check the type of change your PR introduces:

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

It says AGL, but it is actually ASL.

What is the new behavior?

Environment Analysis no longer suffers from ASL/AGL label mistakes

Does this introduce a breaking change?

  • Yes (it modifies documentation of a class)

Other information

It only changes the labels of some charts. Numerical results are kept the same

@Gui-FernandesBR Gui-FernandesBR added this to the Release v1.1.0 milestone Sep 3, 2023
@Gui-FernandesBR Gui-FernandesBR self-assigned this Sep 3, 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 linked an issue Sep 3, 2023 that may be closed by this pull request
@Gui-FernandesBR Gui-FernandesBR changed the base branch from master to beta/v1.0.0 September 5, 2023 12:24
@Gui-FernandesBR Gui-FernandesBR added Bug Something isn't working Environment Enviroment Class related features labels Sep 5, 2023
@phmbressan
Copy link
Collaborator

Good PR, fixes up nomenclature confusion.

As a side note for a seperate docs PR, the functions regarding geopotential calculation and parameters bilinear interpolation would benefit a lot from a update in their docstrings.

@Gui-FernandesBR
Copy link
Member Author

Good PR, fixes up nomenclature confusion.

As a side note for a seperate docs PR, the functions regarding geopotential calculation and parameters bilinear interpolation would benefit a lot from a update in their docstrings.

@phmbressan thks for fast reviewing.

Regarding the documentation... Are we looking to the same versions?
Idk what could be better in the docstrings of the functions you mentioned:

image

image

@MateusStano MateusStano merged commit 6560ccc into beta/v1.0.0 Sep 6, 2023
@MateusStano MateusStano deleted the bug/env-analysis-ASL-mistake branch September 6, 2023 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Environment Enviroment Class related features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: EnvironmentAnalysis altitudes ASL vs AGL
3 participants