Skip to content

BUG: fix standard atmosphere #369

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 1 commit into from
May 29, 2023
Merged

Conversation

Gui-FernandesBR
Copy link
Member

Pull request type

Please check the type of change your PR introduces:

  • Code base additions (bugfix, features)

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

@MateusStano recently caught an inconsistency with the geopotential <=> geometric altitudes in the Environment Class.
I hereby confirm that is currently a BUG and the code has not been working properly so far.

What is the new behavior?

I verified every equation in the Standard Atmosphere model, comparing each of them with the ISO 2533 available online.
All the inconsistencies were removed.
A few comments were provided to warn users of model limitations.

Does this introduce a breaking change?

  • Yes (it modifies a basic calculation regarding environment pressure profile and temperature in ISA. As the difference between geopotential and geometric altitudes are usually lower and 1%, the impact will be very small. It doesn't even affects the current acceptance and unit tests).

Other information

  • It is not hard to find the ISO-2533 online, just google it including "pdf". However, it seems that it was update in 2021 but only a paid version can be found so far: https://www.iso.org/standard/7472.html
  • For future reference: something that really annoys me is that the code consider height as a synonym of altitude. Technically they are different things, and I would agree with a quick name refactoring in the future. As a rule of thumb, whenever we wrote "height", just replace to "altitude" and we will be more accurate.

@Gui-FernandesBR Gui-FernandesBR linked an issue May 26, 2023 that may be closed by this pull request
@Gui-FernandesBR Gui-FernandesBR added this to the Release v1.0.0 milestone May 26, 2023
@Gui-FernandesBR Gui-FernandesBR added Bug Something isn't working Environment Enviroment Class related features labels May 26, 2023
Copy link
Collaborator

@phmbressan phmbressan left a comment

Choose a reason for hiding this comment

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

Good that weird code block was corrected. Just to be sure I checked the results with the module Ambience and they are good.

@MateusStano
Copy link
Member

  • For future reference: something that really annoys me is that the code consider height as a synonym of altitude. Technically they are different things, and I would agree with a quick name refactoring in the future. As a rule of thumb, whenever we wrote "height", just replace to "altitude" and we will be more accurate.

Interesting. We can do this right before or right after (or during I guess...) the snake case changes

@Gui-FernandesBR
Copy link
Member Author

Good that weird code block was corrected. Just to be sure I checked the results with the module Ambience and they are good.

Great idea, @phmbressan ! I never used this Ambience module, could you provide me a link to its project?
I found this one, is the same that you used? https://pypi.org/project/ambiance/

@Gui-FernandesBR
Copy link
Member Author

Gui-FernandesBR commented May 27, 2023

For future reference: something that really annoys me is that the code consider height as a synonym of altitude. Technically they are different things, and I would agree with a quick name refactoring in the future. As a rule of thumb, whenever we wrote "height", just replace to "altitude" and we will be more accurate.
Interesting. We can do this right before or right after (or during I guess...) the snake case changes

Agreed! Therefore I'm mentioning #361 here too

@phmbressan
Copy link
Collaborator

Good that weird code block was corrected. Just to be sure I checked the results with the module Ambience and they are good.

Great idea, @phmbressan ! I never used this Ambience module, could you provide me a link to its project?
I found this one, is the same that you used? https://pypi.org/project/ambiance/

Yes, that one. I made a typo in the module name it is Ambiance.

@Gui-FernandesBR
Copy link
Member Author

Good that weird code block was corrected. Just to be sure I checked the results with the module Ambience and they are good.

Great idea, @phmbressan ! I never used this Ambience module, could you provide me a link to its project?
I found this one, is the same that you used? https://pypi.org/project/ambiance/

Yes, that one. I made a typo in the module name it is Ambiance.

Great, many thank for slid reviewing it.

Based on the two approved reviews, I'm proceeding with the merge operation

@Gui-FernandesBR Gui-FernandesBR merged commit 3c68556 into beta/v1.0.0 May 29, 2023
@Gui-FernandesBR Gui-FernandesBR deleted the bug/standard-atm branch May 29, 2023 04:10
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.

"StandardAtmosphere" environment pressure calculation
3 participants