-
Notifications
You must be signed in to change notification settings - Fork 3
Array based vertical interpolation #85
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #85 +/- ##
===========================================
- Coverage 99.58% 96.97% -2.61%
===========================================
Files 14 21 +7
Lines 962 1556 +594
Branches 15 33 +18
===========================================
+ Hits 958 1509 +551
- Misses 2 42 +40
- Partials 2 5 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| def interpolate_hybrid_to_pressure_levels( | ||
| data: ArrayLike, | ||
| target_p: ArrayLike, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A very small point, but perhaps it is better to put target_p after sp in order to be more consistent in parameter ordering with the other functions - it would make it more consistent when passing positional parameters to these functions.
|
Concerning the hybrid coordinates, I would marginally vote for option A, i.e. do not duplicate the functions in order to specify different kwargs. I think we can/should detect if too much/little information is passed in kwargs and raise an error if the user passes both kinds of information. |
|
Concerning the aux coords, I think the current approach is good - the generic monotonic function is for advanced users with specific needs, and it makes sense for it to have more generic parameters, while the higher-level functions have more 'useful' parameter names because we know the context. |
Description
This PR adds the following array based methods:
Hybrid level (IFS model) computations:
Interpolation:
The following methods become deprecated:
Hybrid coordinates
For hybrid level computations/interpolations the pressure related parameters can be specified in two ways:
So far all the methods uses Option A. The exception is
relative_geopotential_thickness_on_hybrid_levels, this should be revisited though:Problems/questions:
interpolate_hybrid_to_height_levelshas a large number of args/kwargsAux coordinates
The aux data/coord helps interpolation below or above the coordinate range. A typical use case is to interpolate to height levels between the surface and a lowest hybrid (model) level.
Right now, there is an inconsistency in the kwarg names. The generic
interpolate_monotonic()usesaux_min_level_dataandaux_min_level_coord. However, ininterpolate_hybrid_to_height_levels()the corresponding kwags are called asaux_bottom_dataandaux_bottom_h. The reason behind this is that in the generic case we do not now the name and nature of the coordinate so the termsmin/maxandcoordhave to be used. In the concrete case top/bottom have a meaning and the coordinate is already referred to ashin the other kwargs.A decision has to be made whether it is the right approach or just confusing for users.
TODO
These will be sorted out in separate PRs
Contributor Declaration
By opening this pull request, I affirm the following: