Skip to content
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

Feature/eckit geo #275

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Feature/eckit geo #275

wants to merge 3 commits into from

Conversation

pmaciel
Copy link
Member

@pmaciel pmaciel commented Nov 20, 2024

This is an integration of the eckit::geo grids and iterators into eccodes. This needs to be switched at configuration time with both:

  • CMake variables ENABLE_GEOGRAPHY=ON and ENABLE_ECKIT_GEO=ON (configuration time), and
  • env variable ECCODES_ECKIT_GEO=1 (runtime)

in which case any request for a eccodes "geo iterator" will create a eckit::geo:Grid and iterate from it -- that is, there only one eckit::geo-based iterator, which should support all GRIB grids.

This version already supports the corrections to known GRIB problems from a yaml, and some other corrections hardcodes, as fixed in mir (the "GribSpec" class is imported from MIR's "GribInput").

Also, note that not all grids are supported or tested just yet, though the regular_ll, regular_gg, reduced_gg, HEALPix, and similar should work minus some fixes. The projection-based grids not so, there's no link to the "ProjectionSpec" yet.

@pmaciel
Copy link
Member Author

pmaciel commented Nov 20, 2024

I've changed the code to minimise the intrusion in the original code. The building of eckit::geo-based iterators is now placed at the earliest possible call.

@pmaciel
Copy link
Member Author

pmaciel commented Nov 20, 2024

Before anyone asks (!) the change in grib_tools.cc is to call eckit::Main::initialise, a requirement, compiled out in case of disabled feature. It also supports puture packing from eckit (when that happens.)

@shahramn
Copy link
Collaborator

We won't need to call:
eckit::Main::initialise(argc, argv);
in the tools. As the tools will not need to change.
We are only changing the implementation of the src/geo_iterators

@codecov-commenter
Copy link

codecov-commenter commented Nov 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.78%. Comparing base (f4279fc) to head (9f4fd9d).
Report is 7 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop     #275   +/-   ##
========================================
  Coverage    87.78%   87.78%           
========================================
  Files          808      808           
  Lines        62066    62067    +1     
  Branches     11024    11024           
========================================
+ Hits         54484    54485    +1     
  Misses        7582     7582           

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

@pmaciel
Copy link
Member Author

pmaciel commented Nov 21, 2024

We won't need to call: eckit::Main::initialise(argc, argv); in the tools. As the tools will not need to change. We are only changing the implementation of the src/geo_iterators

This is a requirement on using eckit, and is part of its design. Main holds a singleton to access command-line options, and "resources" (specially formatted arguments and environment variables), and is responsible for initialising low-level infrastructure like logging channels. The natural place for it to be is right next to the main() function, hence the change.

But well pointed, I added an initialisation if you trigger the eckit::geo functionality not from a tool that it will do a blng initialisation (ie. initialise eckit, regardless.) We don't have a use for that just yet, but will soon.

@pmaciel
Copy link
Member Author

pmaciel commented Nov 21, 2024

Note that this version already supports ORCA and FESOM grids (!), among other functionality that is difficult to check at the moment, so I can call grib_get_data on a GRIB and it will download the necessary files and run properly.

@shahramn shahramn added the approved-for-ci Approved to run CI on ECMWF machines label Nov 26, 2024
CMakeLists.txt Outdated
DESCRIPTION "Support for Geoiterator and nearest neighbour (additional backend)"
CONDITION ENABLE_GEOGRAPHY
DEFAULT OFF )
if( ECKIT_GEO AND NOT TARGET eckit_geo )
Copy link
Member

Choose a reason for hiding this comment

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

This test did not work for me, so I pushed some changes, which now makes the detection of eckit and test of options work for me. Have a look and see what you think!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved-for-ci Approved to run CI on ECMWF machines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants