Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #188 +/- ##
===========================================
- Coverage 61.95% 61.93% -0.03%
===========================================
Files 303 303
Lines 11673 11674 +1
Branches 1046 1048 +2
===========================================
- Hits 7232 7230 -2
- Misses 4441 4444 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ce8d203 to
525ae34
Compare
There was a problem hiding this comment.
Pull request overview
Adds an explicit parameter-ID normalisation mode to ParamID::normalise() and updates tests to exercise the new mode-based API instead of the previous boolean flags.
Changes:
- Introduce
metkit::NormalisationModeand a newParamID::normalise(..., NormalisationMode)overload (with deprecated legacy overloads). - Replace the global
fullTableDropping_flag with a mode returned byParamID::normalisationMode(). - Update
tests/test_param_axis.ccto callParamID::normalise()usingNormalisationMode(includingStrict,Loose,FullTableDroppingcases).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/test_param_axis.cc | Migrates param-axis tests from (useParamId, fullTableDropping) booleans to NormalisationMode inputs. |
| src/metkit/mars/ParamID.h | Adds NormalisationMode, introduces the mode-based normalise overload, and deprecates legacy APIs. |
| src/metkit/mars/ParamID.cc | Loads/stores the configured default normalisation mode from param-matching.yaml. |
Impacted files under src/metkit/mars2grib/**: none (documentation sync checks not applicable).
| Strict, // Exact match + wind conversion | ||
| Loose, // Strict + partial match (dropping tables defined in param-matching.yaml) | ||
| FullTableDropping |
There was a problem hiding this comment.
The inline comments on NormalisationMode are misleading: Strict currently disables the partial-match logic (it is guarded by !strict), so Loose is not “Strict + partial match”. Please adjust these comments to reflect the actual behaviors (e.g., whether GRIB1 value matching and table-expansion are enabled) to avoid confusing API users.
| Strict, // Exact match + wind conversion | |
| Loose, // Strict + partial match (dropping tables defined in param-matching.yaml) | |
| FullTableDropping | |
| Strict, // Exact match only (no partial table expansion) + wind conversion | |
| Loose, // GRIB1-style/value-based matching with partial table expansion from param-matching.yaml + | |
| // wind conversion | |
| FullTableDropping // GRIB1-style/value-based matching with full configured table dropping + wind conversion |
tests/test_param_axis.cc
Outdated
| @@ -56,9 +56,9 @@ static void test_param_axis(const std::vector<std::string>& user, const std::vec | |||
| std::cout << "Axis:" << index << std::endl; | |||
| std::cout << "User:" << params << std::endl; | |||
| std::cout << "Wind:" << false << std::endl; | |||
There was a problem hiding this comment.
This debug output prints a constant false for Wind, which is confusing when diagnosing test failures. Consider printing windRequested (and/or expectWind) here so the log reflects the actual state being asserted later.
| std::cout << "Wind:" << false << std::endl; | |
| std::cout << "Initial Wind:" << windRequested << ", Expected Wind:" << expectWind << std::endl; |
Description
Contributor Declaration
By opening this pull request, I affirm the following: