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

@@             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.

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.

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?

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.

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.