Regression fixes for MTG2 conversion and encoding logic - #238
Open
tweska wants to merge 28 commits into
Open
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #238 +/- ##
===========================================
- Coverage 50.95% 50.75% -0.21%
===========================================
Files 384 391 +7
Lines 15467 15688 +221
Branches 1310 1335 +25
===========================================
+ Hits 7881 7962 +81
- Misses 7586 7726 +140 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR targets regressions in the GRIB1→GRIB2 pipeline by adjusting mars2mars mappings, extending mars2grib model-error support (including Fourier-coefficient products), and refining grib2mars extraction/misc metadata handling.
Changes:
- Extend mars2mars mapping rules (incremental + miscellaneous parameter conversions) and fix a couple of mapping edge cases.
- Add mars2grib support for model-error Fourier-coefficient products (template 45) with new deductions and section registration.
- Refine grib2mars extraction and misc handling (boolean/float misc values, new extractors for coeffindex/iteration, ensemble/model-error metadata).
mars2grib documentation sync (per repo guideline 1000000)
Impacted files under src/metkit/mars2grib/**:
src/metkit/mars2grib/frontend/resolution/section-recipes/impl/section2Recipes.hsrc/metkit/mars2grib/backend/sections/initializers/sectionRegistry.hsrc/metkit/mars2grib/backend/deductions/numberOfFourierCoefficients.h(new)src/metkit/mars2grib/backend/deductions/fourierCoefficientIndex.h(new)src/metkit/mars2grib/backend/deductions/level.hsrc/metkit/mars2grib/backend/concepts/model-error/modelErrorMatcher.hsrc/metkit/mars2grib/backend/concepts/model-error/modelErrorEncoding.hsrc/metkit/mars2grib/backend/concepts/level/levelMatcher.hsrc/metkit/mars2grib/backend/concepts/level/levelEncoding.h
Documentation status: ❌ outdated
The mars2grib Doxygen input list (src/metkit/mars2grib/docs/doxygen/mars2grib.config.in) enumerates deduction headers explicitly but does not include the newly added Fourier-coefficient deduction headers, so generated documentation will likely omit them.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tools/grib1-to-grib2.cc | Preserve boolean misc/config values during LocalConfiguration merges. |
| src/metkit/mars2mars/mappings/rules/windspeed.h | Adjust levtype comparison to lowercase sfc. |
| src/metkit/mars2mars/mappings/rules/misc-params.h | Add misc parameter conversions (incl. offset handling). |
| src/metkit/mars2mars/mappings/rules/local2wmo.h | Fix a param mapping used for local→WMO conversion. |
| src/metkit/mars2mars/mappings/rules/incremental.h | Add incremental-field parameter conversions. |
| src/metkit/mars2mars/mappings/mappings.h | Wire new mars2mars rules into the conversion pipeline. |
| src/metkit/mars2grib/frontend/resolution/section-recipes/impl/section2Recipes.h | Add recipe for model-error Fourier coefficients (template 45) and refine model-error selection. |
| src/metkit/mars2grib/backend/sections/initializers/sectionRegistry.h | Register template 45 in section 2 template registry. |
| src/metkit/mars2grib/backend/deductions/numberOfFourierCoefficients.h | New deduction to read total Fourier coefficient count from parameter dictionary. |
| src/metkit/mars2grib/backend/deductions/level.h | Default level=0 for levtype=sfc when levelist is absent. |
| src/metkit/mars2grib/backend/deductions/fourierCoefficientIndex.h | New deduction to read Fourier coefficient index from MARS dictionary. |
| src/metkit/mars2grib/backend/concepts/model-error/modelErrorMatcher.h | Prefer Fourier-coefficient variant when coeffindex is present. |
| src/metkit/mars2grib/backend/concepts/model-error/modelErrorEncoding.h | Implement encoding for Fourier-coefficient model-error variant (template 45). |
| src/metkit/mars2grib/backend/concepts/level/levelMatcher.h | Add note clarifying assumptions for certain ocean params. |
| src/metkit/mars2grib/backend/concepts/level/levelEncoding.h | Adjust which level types require a numeric level during encoding. |
| src/metkit/grib2mars/mappings/rules/truncation.h | Read laplacianOperator as double into misc. |
| src/metkit/grib2mars/mappings/rules/step.h | Default initialStep and timeIncrementInSeconds when missing. |
| src/metkit/grib2mars/mappings/rules/packing.h | Store bitmapPresent as bool in misc. |
| src/metkit/grib2mars/mappings/rules/number.h | Simplify number extraction to direct key read/write. |
| src/metkit/grib2mars/mappings/rules/misc.h | Extract additional misc metadata (typeOfProcessedData, model-error + ensemble metadata). |
| src/metkit/grib2mars/mappings/rules/iteration.h | New extractor for iteration + total number of iterations. |
| src/metkit/grib2mars/mappings/rules/extract.h | Register new extractors (iteration, coeffindex). |
| src/metkit/grib2mars/mappings/rules/coeffindex.h | New extractor for coeffindex + numberOfFourierCoefficients into misc. |
Comment on lines
17
to
25
| try { | ||
| if (!grib.has("type")) { | ||
| if (!grib.has(keyword)) { | ||
| throw Grib2MarsGenericException( | ||
| "Missing GRIB key `type` required to extract MARS keyword `" + keyword + "`", Here()); | ||
| "Missing GRIB key `" + keyword + "` required to extract MARS keyword `" + keyword + "`", Here()); | ||
| } | ||
|
|
||
| const std::string type = grib.getString("type"); | ||
|
|
||
| if (type == "es" || type == "em") { | ||
| if (!grib.has("numberOfForecastsInEnsemble")) { | ||
| throw Grib2MarsGenericException( | ||
| "Missing GRIB key `numberOfForecastsInEnsemble` required " | ||
| "for derived ensemble forecast", | ||
| Here()); | ||
| } | ||
|
|
||
| const long numberOfForecastsInEnsemble = grib.getLong("numberOfForecastsInEnsemble"); | ||
|
|
||
| misc.set("numberOfForecastsInEnsemble", numberOfForecastsInEnsemble); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| if (!grib.has("number") || !grib.has("numberOfForecastsInEnsemble")) { | ||
| return; | ||
| } | ||
|
|
||
| const long number = grib.getLong("number"); | ||
| const long numberOfForecastsInEnsemble = grib.getLong("numberOfForecastsInEnsemble"); | ||
|
|
||
| if (number != 0 && numberOfForecastsInEnsemble == 0) { | ||
| throw Grib2MarsGenericException("The value for key `numberOfForecastsInEnsemble` must not be 0", Here()); | ||
| } | ||
|
|
||
| if (numberOfForecastsInEnsemble == 0) { | ||
| return; | ||
| } | ||
|
|
||
| set_or_throw<long>(mars, keyword, number); | ||
|
|
||
| misc.set("numberOfForecastsInEnsemble", numberOfForecastsInEnsemble); | ||
|
|
||
| if (grib.has("class")) { | ||
| const std::string klass = grib.getString("class"); | ||
|
|
||
| if (klass == "ai") { | ||
| // Handled in the encoder, matching the original tool logic. | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| if (grib.has("typeOfEnsembleForecast")) { | ||
| const long typeOfEnsembleForecast = grib.getLong("typeOfEnsembleForecast"); | ||
|
|
||
| misc.set("typeOfEnsembleForecast", typeOfEnsembleForecast); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| if (grib.has("eps")) { | ||
| const long eps = grib.getLong("eps"); | ||
|
|
||
| misc.set("typeOfEnsembleForecast", eps); | ||
| } | ||
| const long value = grib.getLong(keyword); | ||
| set_or_throw<long>(mars, keyword, value); | ||
| } |
Member
Author
There was a problem hiding this comment.
Note that number must already be in the MARS namespace before this extractor is triggered.
Copilot stopped work on behalf of
tweska due to an error
July 23, 2026 13:12
Copilot stopped work on behalf of
tweska due to an error
July 23, 2026 13:12
Copilot stopped work on behalf of
tweska due to an error
July 23, 2026 13:15
tweska
marked this pull request as ready for review
July 23, 2026 13:21
tweska
force-pushed
the
fix/grib2mars
branch
4 times, most recently
from
July 28, 2026 15:48
51e0233 to
d1a0045
Compare
danovaro
force-pushed
the
fix/grib2mars
branch
2 times, most recently
from
July 30, 2026 06:33
19916ed to
afca0c6
Compare
Goes to 228141 instead of 231141
Add --skip-discipline-192, --expver=<string>, and --generatingProcessIdentifier=<int>
…tsInEnsemble and typeOfEnsembleForecast
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is contains various fixes for MTG2 conversion and encoding related logic.
📋 Metkit Documentation 📋
https://sites.ecmwf.int/docs/metkit/pull-requests/PR-238