Draft
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #195 +/- ##
===========================================
- Coverage 61.99% 61.84% -0.16%
===========================================
Files 303 308 +5
Lines 11663 11695 +32
Branches 1048 1050 +2
===========================================
+ Hits 7231 7233 +2
- Misses 4432 4462 +30 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds backend/frontend wiring to support mars.iteration and Local Use Section templates with local definition numbers 20 and 38 in the mars2grib encoder.
Changes:
- Added a new backend
iterationconcept (matcher, enum, concept descriptor, encoding op) and supporting deductions. - Extended Section 2 template recipes to include new templates 20 and 38 and select the new concept accordingly.
- Updated
analysisconcept validation to also allow local definition number 38.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/metkit/mars2grib/frontend/resolution/section-recipes/impl/section2Recipes.h | Adds Section 2 recipes for templates 20 and 38, enabling IterationConcept selection. |
| src/metkit/mars2grib/backend/deductions/totalNumberOfIterations.h | New deduction for totalNumberOfIterations (currently has a confirmed hours→seconds conversion bug + log formatting issue). |
| src/metkit/mars2grib/backend/deductions/iterationNumber.h | New deduction for iterationNumber (Doxygen description currently mismatched). |
| src/metkit/mars2grib/backend/concepts/iteration/iterationMatcher.h | Activates the iteration concept when mars["iteration"] is present. |
| src/metkit/mars2grib/backend/concepts/iteration/iterationEnum.h | Defines iteration concept variant metadata. |
| src/metkit/mars2grib/backend/concepts/iteration/iterationEncoding.h | Implements IterationOp to set iterationNumber / totalNumberOfIterations for LDN 20/38 (Doxygen description currently mismatched). |
| src/metkit/mars2grib/backend/concepts/iteration/iterationConceptDescriptor.h | Registers the new iteration concept into the compile-time registry. |
| src/metkit/mars2grib/backend/concepts/analysis/analysisEncoding.h | Allows analysis concept to run for LDN 38 in addition to 36. |
| src/metkit/mars2grib/backend/concepts/AllConcepts.h | Registers IterationConcept in the global concept list. |
Documentation sync (per mars2grib doc locations):
- Impacted files under
src/metkit/mars2grib/**: the 9 files listed above. src/metkit/mars2grib/docs/**: ✅ in sync (no existing doc sections found that enumerate these specific templates/keys).- In-code Doxygen docs for the new iteration pieces: ❌ outdated (see PR comments on
iterationEncoding.h,iterationNumber.h,totalNumberOfIterations.h).
| // Emit RESOLVE log entry | ||
| MARS2GRIB_LOG_RESOLVE([&]() { | ||
| std::string logMsg = "`totalNumberOfIterations` resolved from input dictionaries: value='"; | ||
| logMsg += std::to_string(totalNumberOfIterationsVal); |
Comment on lines
+136
to
+146
| long totalNumberOfIterationsVal = get_or_throw<long>(par, "totalNumberOfIterations"); | ||
|
|
||
| // Emit RESOLVE log entry | ||
| MARS2GRIB_LOG_RESOLVE([&]() { | ||
| std::string logMsg = "`totalNumberOfIterations` resolved from input dictionaries: value='"; | ||
| logMsg += std::to_string(totalNumberOfIterationsVal); | ||
| return logMsg; | ||
| }()); | ||
|
|
||
| // Success exit point | ||
| return {totalNumberOfIterationsVal}; // Convert hours to seconds |
Comment on lines
+12
to
+17
| /// @file iterationNumber.h | ||
| /// @brief Deduction of the offset to the end of the 4D-Var analysis window. | ||
| /// | ||
| /// This header defines deduction utilities used by the mars2grib backend | ||
| /// to resolve the **offset to the end of the 4D-Var assimilation window** | ||
| /// from input dictionaries. |
Comment on lines
+18
to
+23
| /// The iteration concept is responsible for encoding GRIB keys associated with | ||
| /// *long-range forecast metadata* stored in the Local Use Section, specifically: | ||
| /// | ||
| /// - `methodNumber` | ||
| /// - `systemNumber` | ||
| /// |
| /// @param[in] par Parameter dictionary | ||
| /// @param[in] opt Options dictionary (unused) | ||
| /// | ||
| /// @return The length of time window in seconds. If `par::totalNumberOfIterations` is missing, |
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
Add support for mars.iteration and localSectionNumber=[20,38]
Contributor Declaration
By opening this pull request, I affirm the following: