Conversation
There was a problem hiding this comment.
Pull request overview
Adjusts mars2grib concept matchers to better align ERA6 re-encoding behavior with expected tables/level/point-in-time handling for specific parameter IDs.
Changes:
- Remove the chemical-products special-case that selected
TablesType::Customin thetablesmatcher. - Expand/shift several parameter ranges in the
point-in-timematcher (incl. additional 21x/262xxx codes). - Update
levelmatching for SFC/O2D to align with the updated parameter coverage (incl.262900,262906,262907, and updated 21x ranges).
Impacted files under src/metkit/mars2grib/**:
src/metkit/mars2grib/backend/concepts/tables/tablesMatcher.hsrc/metkit/mars2grib/backend/concepts/point-in-time/pointInTimeMatcher.hsrc/metkit/mars2grib/backend/concepts/level/levelMatcher.h
Documentation status (src/metkit/mars2grib/docs/**): ✅ in sync (no public-behavior documentation found that enumerates/depends on these specific param-ID lists).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/metkit/mars2grib/backend/concepts/tables/tablesMatcher.h | Stops selecting custom tables for chemical params (now always default). |
| src/metkit/mars2grib/backend/concepts/point-in-time/pointInTimeMatcher.h | Adjusts param inclusion ranges (notably 21x and 262xxx additions/expansions). |
| src/metkit/mars2grib/backend/concepts/level/levelMatcher.h | Aligns SFC and O2D level matching with updated param ranges/new ocean-related params. |
| std::size_t tablesMatcher(const MarsDict_t& mars, const OptDict_t& opt) { | ||
| using metkit::mars2grib::util::param_matcher::matchAny; | ||
| using metkit::mars2grib::util::param_matcher::range; | ||
| using metkit::mars2grib::utils::dict_traits::get_or_throw; | ||
|
|
||
| const auto param = get_or_throw<long>(mars, "param"); | ||
| // Chemical products | ||
| if (matchAny(param, range(233032, 233035), range(235062, 235064))) { | ||
| return static_cast<std::size_t>(TablesType::Custom); | ||
| } | ||
|
|
||
| return static_cast<size_t>(TablesType::Default); |
There was a problem hiding this comment.
tablesMatcher() now always returns TablesType::Default, but it still reads param via get_or_throw and keeps matchAny/range using-declarations and the param-matcher include. This adds an unnecessary dictionary access (and can throw if param is absent) despite param no longer influencing the result. Consider removing the param lookup and the now-unused matchAny/range aliases/includes, or otherwise documenting/justifying why param is still required here.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release/1.18.0 #194 +/- ##
=================================================
Coverage ? 62.00%
=================================================
Files ? 303
Lines ? 11662
Branches ? 1049
=================================================
Hits ? 7231
Misses ? 4431
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
d4d4b8d to
9c289fd
Compare
Description
Contributor Declaration
By opening this pull request, I affirm the following: