Feature/era6 conversion support#206
Open
dsarmany wants to merge 15 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends mars2grib’s ERA6/ECMWF conversion support by adding new concept variants and matcher/deduction logic for (a) 4D-var model-error products, (b) “abstractLevel” covariance paramIds, (c) additional O2D salinity paramIds, and (d) PV-array emission suppression for hybrid-level re-encoding.
Changes:
- Add Section 2 local definition 39 support via a new
AnalysisType::ModelErrorsvariant and associated recipe/registry wiring. - Extend level/point-in-time matching to cover new paramId sets (notably 25400x abstractLevel covariance params and 262146..262148 O2D salinity).
- Improve deductions/encoding behavior: infer ensemble generating-process for
type=fcwhen ensemble evidence exists; suppress PV emission when explicitly requested.
mars2grib documentation sync (per repo guideline):
- Impacted files under
src/metkit/mars2grib/**: ✅ in sync (no updates required insrc/metkit/mars2grib/docs/**were identified for these primarily internal mapping changes)
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/mars2grib/backend/mars2grib-test-typeOfGeneratingProcess-deduction.cc | New unit tests for typeOfGeneratingProcess deduction (fc ensemble evidence, etc.) |
| tests/mars2grib/backend/mars2grib-test-o2d-vertically-integrated-salinity.cc | New tests for O2D level + point-in-time mapping for 262146/262147/262148 |
| tests/mars2grib/backend/mars2grib-test-level-pv-suppression.cc | New tests for PV emission suppression in LevelOp allocation stage |
| tests/mars2grib/backend/mars2grib-test-analysis-model-errors.cc | New tests for analysisMatcher selecting ModelErrors for type=eme |
| tests/mars2grib/backend/CMakeLists.txt | Registers the new backend unit tests |
| src/metkit/mars2grib/frontend/resolution/section-recipes/impl/section2Recipes.h | Adds recipe for local def 39; restricts recipe 36 to AnalysisType::Default |
| src/metkit/mars2grib/backend/sections/initializers/sectionRegistry.h | Registers Section 2 template 39 initializer |
| src/metkit/mars2grib/backend/deductions/typeOfGeneratingProcess.h | Adds ensemble-evidence detection for type=fc; maps eme/me to Analysis |
| src/metkit/mars2grib/backend/deductions/significanceOfReferenceTime.h | Adds type=est to the forecast-like type list |
| src/metkit/mars2grib/backend/concepts/point-in-time/pointInTimeMatcher.h | Adds Default point-in-time mapping for 254001..254017 covariance params; adds 262146..262148 |
| src/metkit/mars2grib/backend/concepts/level/levelMatcher.h | Adds SFC mapping for 25400x covariance params to AbstractLevel; extends O2D mapping |
| src/metkit/mars2grib/backend/concepts/level/levelEnum.h | Introduces LevelType::AbstractLevel |
| src/metkit/mars2grib/backend/concepts/level/levelEncoding.h | Implements PV suppression signal via par["PVPresent"]; encodes abstractLevel fixed-surface keys |
| src/metkit/mars2grib/backend/concepts/ensemble/ensembleMatcher.h | Avoids activating Ensemble concept for type=eme where mars.number is componentIndex |
| src/metkit/mars2grib/backend/concepts/analysis/analysisMatcher.h | Adds AnalysisType::ModelErrors selection for type=eme with anoffset |
| src/metkit/mars2grib/backend/concepts/analysis/analysisEnum.h | Adds AnalysisType::ModelErrors variant and naming |
| src/metkit/mars2grib/backend/concepts/analysis/analysisEncoding.h | Adds local definition 39 structural validation + required keys for ModelErrors |
| share/metkit/params.yaml | Adds param list group for 254001..254017 under levtype: sfc |
Comment on lines
95
to
99
| template <class MarsDict_t, class ParDict_t, class OptDict_t> | ||
| std::optional<tables::TypeOfGeneratingProcess> resolve_TypeOfGeneratingProcess_opt( | ||
| const MarsDict_t& mars, [[maybe_unused]] const ParDict_t& par, [[maybe_unused]] const OptDict_t& opt) { | ||
| const MarsDict_t& mars, const ParDict_t& par, [[maybe_unused]] const OptDict_t& opt) { | ||
|
|
||
| using metkit::mars2grib::backend::tables::TypeOfGeneratingProcess; |
There was a problem hiding this comment.
The docstring for resolve_TypeOfGeneratingProcess_opt still states that ParDict_t/par are unused, but the implementation now uses par to detect ensemble evidence for type=fc. Please update the parameter documentation (and the @tparam/@param descriptions) to match the new behavior.
Comment on lines
+145
to
+152
| CASE("typeOfGeneratingProcess: unsupported type (an) -> nullopt (regression)") { | ||
| auto mars = marsWithType("an"); | ||
| eckit::LocalConfiguration par{}; | ||
|
|
||
| auto result = resolve_TypeOfGeneratingProcess_opt(mars, par, kEmptyOpt); | ||
|
|
||
| EXPECT(!result.has_value()); | ||
| } |
There was a problem hiding this comment.
This test suite claims to cover all type cases, but it doesn't include a case for the newly added type=eme/type=me mapping (now resolved to TypeOfGeneratingProcess::Analysis). Please add a regression test for eme (and/or me) to ensure that branch remains covered.
| // (individual ensemble member, PDT=1) as well as with non-ensemble | ||
| // analyses (PDT=0). Without this mapping, PointInTimeConcept is left | ||
| // inactive and Section 4 recipe selection fails with "No matching recipe". | ||
| if (matchAny(param, range(254001, 254018))) { |
There was a problem hiding this comment.
range(254001, 254018) is inclusive in this matcher utility, so this will also match 254018. That disagrees with the comment here (254001..254017) and with the params list added in share/metkit/params.yaml (ends at 254017). Please align the code and documentation (either change the range end to 254017 or update the documented/declared paramId set).
Suggested change
| if (matchAny(param, range(254001, 254018))) { | |
| if (matchAny(param, range(254001, 254017))) { |
Comment on lines
+56
to
+64
| // ECMWF covariance / analysis-uncertainty paramIds (254001..254017). | ||
| // These are point-in-time products living on the abstractLevel | ||
| // (typeOfFirstFixedSurface=254) and are used with MARS type=est | ||
| // (individual ensemble member, PDT=1) as well as with non-ensemble | ||
| // analyses (PDT=0). Without this mapping, PointInTimeConcept is left | ||
| // inactive and Section 4 recipe selection fails with "No matching recipe". | ||
| if (matchAny(param, range(254001, 254018))) { | ||
| return static_cast<std::size_t>(PointInTimeType::Default); | ||
| } |
There was a problem hiding this comment.
New covariance/analysis-uncertainty paramId mapping (254001..254017) was added here, but there is no unit test asserting that these params activate PointInTimeType::Default. Please add a small matcher test case (similar to the O2D tests) to prevent regressions.
| // eccodes/definitions/grib2/localConcepts/{ecmf,era6}/paramId.def with | ||
| // typeOfFirstFixedSurface=254, which maps to the eccodes typeOfLevel | ||
| // concept "abstractLevel". | ||
| if (matchAny(param, range(254001, 254018))) { |
There was a problem hiding this comment.
range(254001, 254018) is inclusive, so this mapping also covers 254018, while the surrounding comment and share/metkit/params.yaml list only 254001..254017. Please align the range end with the intended paramId set (or update the declared set accordingly).
Suggested change
| if (matchAny(param, range(254001, 254018))) { | |
| if (matchAny(param, range(254001, 254017))) { |
Comment on lines
+144
to
+150
| // ECMWF covariance paramIds (254001..254017) are defined in | ||
| // eccodes/definitions/grib2/localConcepts/{ecmf,era6}/paramId.def with | ||
| // typeOfFirstFixedSurface=254, which maps to the eccodes typeOfLevel | ||
| // concept "abstractLevel". | ||
| if (matchAny(param, range(254001, 254018))) { | ||
| return static_cast<std::size_t>(LevelType::AbstractLevel); | ||
| } |
There was a problem hiding this comment.
New LevelType::AbstractLevel mapping for covariance params (254001..254017) was added here, but there is no unit test asserting this behavior. Please add a matcher/encoding test to ensure these paramIds keep resolving to AbstractLevel (and, if applicable, that encoding sets typeOfLevel=abstractLevel).
Comment on lines
+264
to
+273
| // Accept either bool or long (eccodes-style 0/1) here: | ||
| // multio's GribKeys defines PVPresent as bool, but other | ||
| // callers may pass a long. dict_traits::get<bool> covers | ||
| // both via the eckit::LocalConfiguration adapter. | ||
| try { | ||
| emitPv = get_or_throw<bool>(par, "PVPresent"); | ||
| } | ||
| catch (...) { | ||
| emitPv = (get_or_throw<long>(par, "PVPresent") != 0L); | ||
| } |
There was a problem hiding this comment.
The comment says dict_traits::get<bool> covers both bool and long via the eckit::LocalConfiguration adapter, but the implementation actually relies on a try/catch fallback to read a long. Please adjust the comment to match reality (or refactor to a clearer typed read) so future readers don't assume the bool getter handles 0/1 longs.
dsarmany
force-pushed
the
feature/era6-conversion-support
branch
from
f9abaf9 to
356915d
Compare
- Add new iteration concept under backend/concepts/iteration/ following the standard 4-file layout (Enum, Matcher, Encoding, ConceptDescriptor); single Default variant, applicable at (StagePreset, SecLocalUseSection), with LocalDefinitionNumber allow-list {20, 38}; encoder sets iterationNumber and (optionally) totalNumberOfIterations.
- Add the supporting deductions backend/deductions/iterationNumber.h (resolve_IterationNumber_or_throw) and backend/deductions/totalNumberOfIterations.h (resolve_TotalNumberOfIterations_opt) used by IterationOp.
- Register IterationConcept in AllConcepts.h: include added alphabetically between generating-process and level; type appended to the AllConcepts typelist after LongrangeConcept.
- Implement iterationMatcher: returns IterationType::Default when mars has the "iteration" key, MISSING otherwise.
- Extend analysisEncoding.h LocalDefinitionNumber allow-list from {36} to {36, 38} so AnalysisOp accepts the new combined template 38 (4i analysis products).
- Add Section 2 recipes in section2Recipes.h: S2_R20 (Mars + Iteration) and S2_R38 (Mars + Iteration + Analysis); both registered in the Section2Recipes aggregator in numerical order.
- Add new modelError concept under backend/concepts/model-error/ following the standard 4-file layout (Enum, Matcher, Encoding, ConceptDescriptor); single Default variant, applicable at (StagePreset, SecLocalUseSection), with LocalDefinitionNumber allow-list {25, 39}; encoder body left as TODO (deductions / GRIB key writes to be added separately).
- Register ModelErrorConcept in AllConcepts.h: include added alphabetically between mars and nil; type appended at the end of the AllConcepts typelist to preserve existing conceptIds and global variant indices.
- Implement modelErrorMatcher: returns ModelErrorType::Default when mars["type"] == "eme"; throws Mars2GribMatcherException if the mandatory "number" key is missing in that case; returns MISSING otherwise.
- Add Section 2 recipes in section2Recipes.h: S2_R25 (Mars + ModelError) and S2_R39 (Mars + Analysis + ModelError); both registered in the Section2Recipes aggregator in numerical order.
- Extend analysisEncoding.h LocalDefinitionNumber allow-list from {36, 38} to {36, 38, 39} so AnalysisOp accepts the new template 39.
- Update ensembleMatcher.h to return MISSING when mars["type"] == "eme", since in that case the "number" key identifies the model-error realization, not an ensemble member.
…MultipleLevel - Replace the single LevelType::Hybrid variant with two variants: ModelSingleLevel for 2D fields published on the model-level system (no vertical column, no PV array) and ModelMultipleLevel for full vertical columns of model-level data, which require allocation and population of the PV array describing the hybrid coordinate transformation. Both variants map to GRIB typeOfLevel "hybrid", so encoded output is bit-identical for cases that previously used Hybrid; only encoder behaviour (PV allocation) differs between the two new variants. - Update the LevelList typelist to reflect the new variants and keep it in sync with the LevelType enumeration. - Update needPv to fire only on ModelMultipleLevel; update needLevel to cover both ModelSingleLevel and ModelMultipleLevel. - Add a new AbstractLevel variant carrying a numeric level value, sitting alongside the existing AbstractSingleLevel and AbstractMultipleLevel opaque variants. AbstractLevel is included in needLevel. - Rewrite matchML to dispatch single-level model paramIds (22, 127, 128, 129, 152) to ModelSingleLevel and the remaining multi-level set to ModelMultipleLevel. ERA6 paramIds 127 and 128 on ML, which were previously rejected, are now accepted as ModelSingleLevel. - Refresh Doxygen for the level concept to document the three orthogonal predicates needPv, needLevel, needTopBottomLevel and to describe the rationale for splitting Hybrid into single-level and multi-level model variants. - Fix typo in the level encoder header comment: "Se of typeOfLevel" becomes "Setting of typeOfLevel".
…fc detection, model-error types)
- params.yaml: register ECMWF covariance paramIds 254001..254017 on levtype=sfc.
- levelMatcher: map paramIds 254001..254017 to LevelType::AbstractLevel
(typeOfFirstFixedSurface=254); extend matchO2D with ocean paramIds
262146/262147 (DepthBelowSeaLayer) and 262148 (OceanSurfaceToBottom).
- pointInTimeMatcher: add 254001..254017 and 262146..262148 to the default
point-in-time set so Section 4 recipe selection succeeds for these params.
- significanceOfReferenceTime: recognize MARS type "est" as a forecast type.
- typeOfGeneratingProcess:
* For type=fc, detect ensemble evidence (numberOfForecastsInEnsemble>1,
typeOfEnsembleForecast present, or mars.number>0) and resolve to
EnsembleForecast instead of Forecast; default behavior is preserved
when no ensemble evidence is present. Adds detail to RESOLVE log.
* Map type=eme/me (4D-Var model errors) to Analysis, matching the
existing {4i,4v,me,eme} grouping in significanceOfReferenceTime.
dsarmany
force-pushed
the
feature/era6-conversion-support
branch
from
356915d to
d0bc20f
Compare
…types (est, es, em, ses)
Parameters 254003/254006/254009 (covar_ssm_swvl1/2/3) require typeOfSecondFixedSurface=151 (soil level) with the corresponding layer number. The ecmf abstractLevel concept forces secondSurface=255, so we explicitly override it for these params. This is a transitional workaround until eccodes supports AbstractSingleLevel and AbstractMultipleLevel level types.
…ed products Fix brightness temperature (btmp) re-encoding for em/es/ses types: - Move paramId=194 from Surface to EntireAtmosphere level type - Relax satellite matcher to activate on param=194+channel (without requiring ident/instrument which are absent in PDT=2 input) - Add S4_R2_SAT recipe for PDT=2 with SatelliteConcept so template selector resolves derived+satellite combination - Make satellite section 4 encoding a no-op for non-32/33 PDTs (satellite metadata lives in section 2 local def 37 only)
- Register template 37 in Sec2Reg section initializer registry - Accept localDefinitionNumber=37 in analysis encoding validation - Map type=ses to DerivedForecast::SpreadAllMembers
dsarmany
force-pushed
the
feature/era6-conversion-support
branch
from
d0bc20f to
3396f27
Compare
…le factors The eccodes concept for 'abstractLevel' requires scaleFactorOfFirstFixedSurface and scaledValueOfFirstFixedSurface to be MISSING. Setting 'level' after 'typeOfLevel=abstractLevel' was overriding those values to 0, causing grib_compare mismatches against correctly stored input data.
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.
Description
Contributor Declaration
By opening this pull request, I affirm the following: