ENH Precalculate Barometric Height

[MateusStano]

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
• CHANGELOG.md has been updated (if relevant)

Current behavior

The find_input calls to get the height AGL from the pressure value for the parachute trigger was slowing down the simulation drastically:

New behavior

Now a Function is created in Environment called barometric_height and that is called inside the Flight class instead:

Additional information

This find_input call was added in #345

[codecov]

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (821518a) 72.34% compared to head (2086996) 72.36%.

❗ Current head 2086996 differs from pull request most recent head d690f15. Consider uploading reports for the commit d690f15 to get more accurate results

Files	Patch %	Lines
rocketpy/environment/environment.py	57.14%	9 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #511      +/-   ##
===========================================
+ Coverage    72.34%   72.36%   +0.02%     
===========================================
  Files           56       56              
  Lines         9393     9409      +16     
===========================================
+ Hits          6795     6809      +14     
- Misses        2598     2600       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Gui-FernandesBR]

Good work, @MateusStano !! Before approving this one, I'd like to see some unit tests for the barometric height, lemme know if you need any help.

Question: What would happen if a given Environment had the same value of pressure at diferent altitude? For example, for really high altitudes.

[MateusStano]

Hey! @Gui-FernandesBR solved everything you commented.

However, during the test creation, I identified an issue with the barometric height data. In some environmental models, the pressure remains constant from the surface up to a specific altitude. For example, the pressure assumed in our tests remains constant from ground level to 722 meters.

This is a problem for the barometric_height calculation, as it results in a constant height for certain pressures. This can mess up some parachute triggers. For instance, using the pressure data from our tests yields a barometric_height function with a minimum height of 722 meters. This inconsistency can disrupt a parachute trigger set at 400 meters, because the function never reaches that value.

To solve this, I've added exatrapolation into the pressure data. I would appreciate a double check on this solution.

[Gui-FernandesBR]

I know the implementation seems to be straightforward, but I'd like to ensure the code will have the best behavior under different circumstances...

1. Somethings isn't right in this plot:

2. What would happen if someone sets the elevation higher than the first available point? e.g. you have pressure starting from 400m but you set the elevation as 500m. (EDIT: Actually I understood now that you set the pressure at the 0 height level, which is not always the ground elevation. Am I right?).

3. When we use ensembles, different pressure profiles are available, how does the barometric_height interact with that? If I change the selcted ensemble member, will the barometric pressure change as well? This is important when running monte carlo

4. I thought about an alternative solution here: Try to set the barometric_height's extrapolation method to "natural" rather than the default "constant". This will make the Function have equivalent behavior as in your code. Why is this a better solution? Less code lines, so less chance to get the things wrong.

5. Something that I'm concerned here is that the pressure and the barometric_height might become incompatible somehow. Meaning that the inverse of barometric_height may not necessarly correspond to the pressure profile, as well as the inverse of pressure may not correspond exactly to the barometric_height. Do you see something similar happening here?

[MateusStano]

Hey, guess I forgot the extrapolation="natural" was a thing heheh

I changed the implementation to use this, and it seems more consistent. The error showed by @Gui-FernandesBR has been fixed.

Regarding the "correctness" of the barometric_height, it should not be very wrong. The barometric_height is essentially calculated in two possible ways. The first is by simply inverting the data used to define the pressure directly, meaning the barometric_height is as "correct" as the pressure curve. The other way is using the ISA modeling, by inverting the ISA pressure model, which is pretty simple so it should not lead to big errors.

The only inconsistency happens with models that consider the pressure to be constant from height 0 to a certain level, like this:

This is using the Wyoming Upper Air Soundings example from the environment usage notebook.

Since we are dealing with parachutes that commonly have triggers at low altitudes, simply inverting this curve would not be sufficient for our applications. The resulting barometric_height of this is model is:

By using extrapolation="natural" the barometric_height stays consistent.

Note that, in this case, the reason for the pressure to be constant below 720m is because the data is from a weather observation site located at an elevation of 720m ASL.

Also note that the height here can reach negative values. However, unless the user has set the elevation of the environment to be negative, this will never be called in the simulation.

The extrapolation is the most logical way to extend the data from environment models to our use case.

[Gui-FernandesBR]

Hey, guess I forgot the extrapolation="natural" was a thing heheh

I changed the implementation to use this, and it seems more consistent. The error showed by @Gui-FernandesBR has been fixed.

Regarding the "correctness" of the barometric_height, it should not be very wrong. The barometric_height is essentially calculated in two possible ways. The first is by simply inverting the data used to define the pressure directly, meaning the barometric_height is as "correct" as the pressure curve. The other way is using the ISA modeling, by inverting the ISA pressure model, which is pretty simple so it should not lead to big errors.

The only inconsistency happens with models that consider the pressure to be constant from height 0 to a certain level, like this:

This is using the Wyoming Upper Air Soundings example from the environment usage notebook.

Since we are dealing with parachutes that commonly have triggers at low altitudes, simply inverting this curve would not be sufficient for our applications. The resulting barometric_height of this is model is:

By using extrapolation="natural" the barometric_height stays consistent.

Note that, in this case, the reason for the pressure to be constant below 720m is because the data is from a weather observation site located at an elevation of 720m ASL.

Also note that the height here can reach negative values. However, unless the user has set the elevation of the environment to be negative, this will never be called in the simulation.

The extrapolation is the most logical way to extend the data from environment models to our use case.

Thanks for the comments! Gonna review it again as soon as possible just to double check it.

[Gui-FernandesBR]

Good work here, @MateusStano !

I ran some examples locally and everything seems to be working correctly.
I'm also trusting the unit tests here.

This PRs reminds me that the Environment class has some code repetitions. We could work on a refactor/overhaul in the future. Lot of the code remains the same since the first commits.