eckit
eckit copied to clipboard
ecmwf
Feature/geo
This PR provides minimal functionality for a grid library, under eckit::geo and resulting in libeckit_geo.dylib. It is meant as a (much) larger superset of the eckit::geometry library which it intends to replace, as well as a very large rewrite.
It has basic support for the following:
- regular ll, regular gg, reduced gg, ORCA, HEALPix grids (ring & nested); They can
- iterate (inside eg. a for loop)
- generate a list of points (a pair of lat/lon vectors, or a single list of points)
- generate a spec ("gridspec"), which can be reused to recreate the same grid
- be instantiated from a spec ("gridspec")
- regular ll, regular gg, reduced gg grids can crop
- the framework for Projections, Figure (shape and size), is in place
- all new class hierarchies are fully adequate for Python interfaces (if need be), that is, the classes are components
Everything is done "as late as possible", and the expensive calculations are cached. There is sufficient support for ORCA grids cached to/from disk to support the existing stack.
What's not there:
- ordering (GRIB scanningMode) handling ~~(incl. HEALPix ordering: nested but that is simple to add now)~~
- projections (incl. rotations), there is heavy testing done already (so it is simple to add now, one-by-one)
This is meant as a starting point. If one aims for full complete integration with eccodes, I would say we are 75% of the way there -- but the last % take longer.
This considers eccodes + atlas + mir + multio (the ORCA and HEALPix parts) + earthkit-regrid. There is also a parallel effort that can trigger most of the above functionality from eccodes, which has been considered for this design so it is suitable, with an equivalent grib_get_data (and an additional grib_get_data_diff) already supporting integration into eccodes (the first project with eckit::geo upstream of eccodes).
Codecov Report
Attention: Patch coverage is 75.25601% with 1184 lines in your changes missing coverage. Please review.
Project coverage is 63.67%. Comparing base (
3a95f91) to head (784b4dc). Report is 23 commits behind head on develop.
Additional details and impacted files
@@ Coverage Diff @@
## develop #116 +/- ##
===========================================
+ Coverage 61.98% 63.67% +1.69%
===========================================
Files 900 1068 +168
Lines 49799 55320 +5521
Branches 3745 4097 +352
===========================================
+ Hits 30866 35223 +4357
- Misses 18933 20097 +1164
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Hi @pmaciel great job for landing this! 💪💪💪 I will need a little bit of time to go through, and probably will review it in chunks at a time if you don't mind.
Is it the intention to remove the feature CONVEX_HULL ?
No, I accidentaly removed it when splitting branches. Thanks for spotting
OK, thanks. There's still a change in how Qhull is detected after your last commit. Probably best to leave it as it was, without "REQUIRED_PACKAGES", and use find_package(Qhull) further. If a change is warranted, best in a separate PR.
OK, thanks. There's still a change in how Qhull is detected after your last commit. Probably best to leave it as it was, without "REQUIRED_PACKAGES", and use
find_package(Qhull)further. If a change is warranted, best in a separate PR.
No it's fine, you're right. I had the right code, but I forked from a while back and did the same with qhull too -- so it was confusing. Now fixed
@pmaciel , is there a gridspec json schema available to test the validity of the input gridspecs?
Private downstream CI failed. Workflow name: private-downstream-ci-hpc View the logs at https://github.com/ecmwf-actions/private-downstream-ci/actions/runs/9019461333.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Private downstream CI succeeded. Workflow name: private-downstream-ci View the logs at https://github.com/ecmwf-actions/private-downstream-ci/actions/runs/9019457090.
@pmaciel , is there a gridspec json schema available to test the validity of the input gridspecs?
@sandorkertesz not yet :-) But the unit tests should be a guide to write one. I've made the whole procedure automatic (eg., it derives a bounding box from a single coordinate, like just "west: 123" to north/west/south/east: 90/123/-90/123+360, or handles a octahedral reduced gg from a single key "N: 64")
We defintely should sit down and write a table of rules -- the ones I've written seem quite sufficient so far and are only maybe 5. This branch can already generate gridspec's if you build a Grid object, and the proof of fire will come first from mir (which will integrate nicelly with the "interpolationspec" already there), and afterwards from eccodes which will have a really comprehensive set of grids (historical, contemprary and wrongly encoded too! :-) )
I've managed to go through everything, although I cannot claim that I was super careful everywhere with such large PR. I made all comments or suggestions in code.
My only real concern is the general Point class as std::variant
* the extra memory * the (little) boiler plate needed to access the correct alternative * the introspection needed to know (or overhead to assert) what's insideI do understand the reason in creation of a more generic Projection API, or part of an iterator where the memory argument is mute, and I could definitely accept that there, but not when it is used in containers such as std::vector, or maps or trees, ... I don't understand why UnstructuredFromGrid cannot be initialised with a vector of
PointLonLatinstead ofPoint, as that is what it is expecting. Also that means any grid could just have the functionto_pointlonlatinstead ofto_pointsthen, which reduces memory by half, possibly important at high resolution.
@wdeconinck Yes the idea here is to discuss the scope -- I believe the fundamental classes are written, and now I'm specialising (and fixing things of course). There's already been this week a "proof by fire" where we compared the performance of eccodes iterator to this one, and they are in the same ballpark (1%-2% performance difference either way), and the results matched down to several decimal places.
The Point class is fundamental here, yes. The point of using it as a variant and not inheritance is to: 1) benefit from the polymorphism without the virtual table penalty, 2) there's no interest in further specialising the point types (if you give it some thought), and 3) enforce checkling of types by the projections themselves, which actually need it specifically enforced (we would otherwise live with several overriden coordinates calculation functions, mostly empty or NOTIMP, and would still need to test dynamic_casts (which I'm alergic to). The cost here is a check on an integer, at the expense of inheritance which we don't need (but is still possible witha variant, but I see absolutely no interest in this library, maybe for projections?), and this check is both trivial and performant.
Performance also is not hindered by this choice: a spectral transfomrm will always win. In pgen the iterator is costly because the whole workflow is optimised, hence these numbers show there (I've measured it at 5%-8% a few years ago), but this library as really fast way to calculate grid sizes (which is what both MIR and eccodes actually needs).
There's a penalty but for performance concerns there's no issue, as these would never explicitly calculate vector<Point> (which yes incurrs on overhead) but rather use the iterator explicitly which is random accessso it can be present in OMP loops or MPI Ranks. The (larger) functions there are for prototyping tests, non-performance critical functions and interfacing with Python really.
This looks really nice overall, despite the number of small comments.
* You seem to really like things declared with __ at the start. This is supposed to be reserved, and I think all of these can just be put into an anonymous namespace * Some of your types are returned with raw pointers. They either need [[nodiscard]], to return a std::unique_ptr, or to have a Value-like wrapper class that handles the pointer stuff internally. * There are a couple of bits (commented) where I don't think the changes are geo-specific. And they could do with going elsewhere. Particularly in spec, I wonder if we could do well to revisit the Parametrisation stuff in general.
@simondsmart thanks for reviewing! Yes my pattern is to start static variables with double underscore (just like in python, to stay clear); I've taken the advice and did the other Python way (or, the Fortran way, bless) all uppercase.
Yes @wdeconinck had the same concern; I don't find this a problem, and I agree with the semantics of returning naked pointers, like I said it is for the client code to care for it. But, the [[nodiscard]] is exactly for this like you mention, so I've changed. No Value-like classes please (yuck).
Yes the parametrisation is a challenge. I need to explicitly control the conversion of types, including vector types, and I cannot afford to rely on Value. -- it just isn't either performant, complete, consistent nor adequate for low level (eg. impossible to efficiently use over a grib handle). So I've written my own again based on a variant. It solves all these problems at the moment, and the only thing it cannot do is handle vectors of more than one type (we don't have this case here) nor nest configurations -- also not the case here, but trivial to add without again resorting the 2 levels of inheritance native to Value.
So there's the Spec class which supports these at the moment -- the challenge was and is ec codes. This implementation is currently adequate, but feel free to dig deeper, I'm sure the work here is not finished, nor is the a solution for everything, just for eckit::geo.
Private downstream CI failed. Workflow name: private-downstream-ci-hpc View the logs at https://github.com/ecmwf-actions/private-downstream-ci/actions/runs/9022911618.
Private downstream CI succeeded. Workflow name: private-downstream-ci View the logs at https://github.com/ecmwf-actions/private-downstream-ci/actions/runs/9022982982.
Private downstream CI succeeded. Workflow name: private-downstream-ci View the logs at https://github.com/ecmwf-actions/private-downstream-ci/actions/runs/9022982982.
Private downstream CI failed. Workflow name: private-downstream-ci-hpc View the logs at https://github.com/ecmwf-actions/private-downstream-ci/actions/runs/9034296445.
@pmaciel - not to be looked at until next week, but I enabled this feature (ECKIT_GEO) on the downstream-ci, which is why the builds are now failing - compilation errors on different platforms.
I temporarily enabled PROJ, but have now disabled it because none of our Linux runners have version 9.2 installed (nor is it available for any of the distributions). I checked, and here are the versions of proj that we have installed on the runners (note: many of these are quite old distros now, and near end of maintenance, so we may drop them soon). Probably most of the proj versions could be accommodated, but the one in centos 7.9 could be a problem...
proj os 8.2 ubuntu 22 7.2 debian 11 4.2 centos 7.9 6.3 rocky 8.6 9.0 Fedora 37
Private downstream CI failed. Workflow name: private-downstream-ci-hpc View the logs at https://github.com/ecmwf-actions/private-downstream-ci/actions/runs/9110966834.
Private downstream CI failed. Workflow name: private-downstream-ci-hpc View the logs at https://github.com/ecmwf-actions/private-downstream-ci/actions/runs/9173312048.
Private downstream CI failed. Workflow name: private-downstream-ci-hpc View the logs at https://github.com/ecmwf-actions/private-downstream-ci/actions/runs/9188769745.
Private downstream CI failed. Workflow name: private-downstream-ci-hpc View the logs at https://github.com/ecmwf-actions/private-downstream-ci/actions/runs/9194956138.
Private downstream CI failed. Workflow name: private-downstream-ci-hpc View the logs at https://github.com/ecmwf-actions/private-downstream-ci/actions/runs/9199214392.
Private downstream CI failed. Workflow name: private-downstream-ci-hpc View the logs at https://github.com/ecmwf-actions/private-downstream-ci/actions/runs/9203874131.
Private downstream CI failed. Workflow name: private-downstream-ci-hpc View the logs at https://github.com/ecmwf-actions/private-downstream-ci/actions/runs/9205363171.
Private downstream CI failed. Workflow name: private-downstream-ci-hpc View the logs at https://github.com/ecmwf-actions/private-downstream-ci/actions/runs/9208363348.
Private downstream CI failed. Workflow name: private-downstream-ci-hpc View the logs at https://github.com/ecmwf-actions/private-downstream-ci/actions/runs/9214927568.
Private downstream CI failed. Workflow name: private-downstream-ci-hpc View the logs at https://github.com/ecmwf-actions/private-downstream-ci/actions/runs/9290156747.
Private downstream CI succeeded. Workflow name: private-downstream-ci View the logs at https://github.com/ecmwf-actions/private-downstream-ci/actions/runs/9294204752.
Private downstream CI succeeded. Workflow name: private-downstream-ci View the logs at https://github.com/ecmwf-actions/private-downstream-ci/actions/runs/9294204752.