Feature/inheritance #85

danovaro · 2025-05-14T13:06:05Z

This PR introduces:

strict validation is enforced by default
introduced aliases for the verbs
ability to specify subtypes and a context to asses which subtype has to be used (to handle tigge)
metadata validation order is based on the canonical axis order, so that metadata used to match the context are already validated
improved date parsing
many tests derived from the mars-client migration suite (@tbkr)

… + expected result

…e of errors

…i.e. tigge)

codecov-commenter · 2025-05-14T14:12:53Z

Codecov Report

Attention: Patch coverage is 91.13924% with 56 lines in your changes missing coverage. Please review.

Project coverage is 66.95%. Comparing base (6b41e2f) to head (11d1550).

Files with missing lines	Patch %	Lines
src/metkit/mars/Type.cc	74.57%	15 Missing ⚠️
tests/test_expand.cc	93.61%	15 Missing ⚠️
src/metkit/mars/MarsLanguage.cc	87.27%	7 Missing ⚠️
src/metkit/mars/TypeDate.cc	83.33%	6 Missing ⚠️
tests/test_request.cc	88.00%	3 Missing ⚠️
src/metkit/mars/Type.h	92.59%	2 Missing ⚠️
src/metkit/mars/TypeEnum.cc	90.00%	2 Missing ⚠️
tests/test_date.cc	91.30%	2 Missing ⚠️
src/metkit/mars/MarsRequest.cc	95.23%	1 Missing ⚠️
src/metkit/mars/Parameter.cc	0.00%	1 Missing ⚠️
... and 2 more

Additional details and impacted files

@@             Coverage Diff             @@
##           develop      #85      +/-   ##
===========================================
+ Coverage    65.90%   66.95%   +1.04%     
===========================================
  Files          122      123       +1     
  Lines         6983     7244     +261     
  Branches       667      707      +40     
===========================================
+ Hits          4602     4850     +248     
- Misses        2381     2394      +13

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Ozaq · 2025-05-20T17:45:33Z

src/metkit/mars/ContextRule.h

+
+//----------------------------------------------------------------------------------------------------------------------
+
+ContextRule::ContextRule(const std::string& k) : key_(k) {}


Its confusing that ContextRule is defined in Type.h and the ContextRule CTor in ContextRule.h

Ozaq · 2025-05-20T17:46:28Z

src/metkit/mars/ContextRule.h

+
+//----------------------------------------------------------------------------------------------------------------------
+
+class Include : public ContextRule {


Please add one sentence explaining what this class represents.

Ozaq · 2025-05-20T17:46:47Z

src/metkit/mars/ContextRule.h

+    std::set<std::string> vals_;
+};
+
+class Exclude : public ContextRule {


Please add one sentence explaining what this class represents.

Ozaq · 2025-05-20T17:47:10Z

src/metkit/mars/ContextRule.h

+    std::set<std::string> vals_;
+};
+
+class Undef : public ContextRule {


Please add one sentence explaining what this class represents.

Ozaq · 2025-05-20T17:47:31Z

src/metkit/mars/ContextRule.h

+    void print(std::ostream& out) const override { out << "Undef[key=" << key_ << "]"; }
+};
+
+class Def : public ContextRule {


Please add one sentence explaining what this class represents.

Ozaq · 2025-05-20T17:50:14Z

src/metkit/mars/MarsExpansion.cc

-    for (const auto& request : requests) {
-        auto& lang = language(request, request.verb());
-        result.emplace_back(lang.expand(request, request, inherit_, strict_));
+    for (size_t i = 0; i < requests.size(); i++) {


Unless I am not mistaken this could still be a range based for loop

Ozaq · 2025-05-20T17:52:15Z

src/metkit/mars/MarsLanguage.cc

@@ -183,7 +193,8 @@ std::string MarsLanguage::bestMatch(const MarsExpandContext& ctx, const std::str
    size_t score = (fullMatch ? name.length() : 1);
    std::vector<std::string> best;

-    static bool strict = eckit::Resource<bool>("$METKIT_LANGUAGE_STRICT_MODE", false);
+
+    static bool strict = eckit::Resource<bool>("$METKIT_LANGUAGE_STRICT_MODE", true);


Does this have downstream impact? If not, why not remove the whole option and always be strict?

Ozaq · 2025-05-20T17:58:14Z

tests/test_expand.cc

+    // {
+    //     const char* text = "retrieve,class=d1,dataset=extreme-dt,date=-1";
+    //     ExpectedVals expected{{"class", {"d1"}},    {"dataset", {"extremes-dt"}},
+    //                           {"expver", {"0001"}}, {"levelist", {"1000", "850", "700", "500", "400", "300"}},
+    //                           {"levtype", {"pl"}},  {"param", {"129"}},
+    //                           {"step", {"0"}},      {"stream", {"oper"}},
+    //                           {"time", {"1200"}},   {"type", {"an"}}};
+    //     expand(text, "retrieve", expected, {-1});
+    // }


Fix or remove

Ozaq · 2025-05-20T18:03:20Z

tests/CMakeLists.txt

@@ -34,6 +34,8 @@ ecbuild_get_test_multidata( TARGET metkit_get_odb_data
 #                   TEST_DEPENDS  grib_get_data
 # )

+file(COPY "${CMAKE_CURRENT_SOURCE_DIR}/expand" DESTINATION "${CMAKE_CURRENT_BINARY_DIR}")


I think this should be a softlink instead. Use

add_custom_command(soft_link_expand_test_data ALL COMMAND ${CMAKE_COMMAND} -E create_symlink ${CMAKE_CURRENT_BINARY_DIR} "${CMAKE_CURRENT_SOURCE_DIR}/expand")

Ozaq · 2025-05-21T06:54:57Z

src/metkit/mars/MarsLanguage.cc

+        // for (std::vector<std::string>::const_iterator k = params.begin(); k != params.end(); ++k) {
+        //     type(*k)->finalise(ctx, result, strict);
+        // }


Please remove commented out code

Ozaq · 2025-05-21T06:56:02Z

src/metkit/mars/ContextRule.h

@@ -0,0 +1,105 @@
+/*
+ * (C) Copyright 1996- ECMWF.


Ozaq · 2025-05-21T06:58:10Z

tests/test_context.cc

@@ -0,0 +1,57 @@
+/*
+ * (C) Copyright 1996- ECMWF.


should be 2025

Ozaq · 2025-05-21T07:21:58Z

tests/test_expand.cc

-            EXPECT_EQUAL(e.second.size(), vv.size());
+    ExpectedRequest() = default;
+
+    ExpectedRequest(std::string vrb, ExpectedVals vv, std::vector<std::string> dd) : verb(vrb), vals(vv), dates(dd) {}


I know it is just test code but all 3 arguments should be moved

Ozaq · 2025-05-21T07:57:28Z

share/metkit/language.yaml

+          - [kwbc]
+          - [none]
+          - [hrm]
+          - [ifs, IFS with no ocean model]


Is there a specific reason 'IFS' and the other all capital letter aliases has been removed?

We aim to handle all requests on a case-insensitive basis, so the code is already handling the enum values disregarding their case.

Ozaq · 2025-05-21T07:58:41Z

share/metkit/language.yaml


  # verify
  # refdate

  hdate:
    category: data
    multiple: true
-    type: integer
+    # type: integer


this should be removed

Ozaq · 2025-05-21T08:25:15Z

share/metkit/param-matching.yaml

@@ -13,3 +13,4 @@ wind:
 # list of tables used by ParamID normalisation to limit the param-table expansion to wind, ocean and reanalysis
 drop-tables: [140,150,151,160,162,170,180,190]
 full-table-dropping: true
+ml-params-single-level: [22, 127, 128, 129, 152]


I am not sure if I understand the purpose but from MarsRequest::count I transcribed that this list of parameters can be used in the presence of a levelist keyword but still result in only a single field. Isn't that a violation of our mars tree design?

Unfortunately, you are right. This violates the assumption that the hypercube is a dense set.
There are a few parameters that are only encoded at levelist=1 for levtype=ml (which is even more odd, since levelist=1 is in the stratosphere).

We suggested encoding those parameters as levtype=sfc or sol, but we have been told that users need to retrieve those parameters together with the 3D of the atmosphere... and we cannot mix different levtypes in a single request.

Ozaq · 2025-05-21T08:26:01Z

src/metkit/mars/MarsRequest.cc

+    size_t levels            = 1;
+    size_t params            = 0;
+    size_t paramsSingleLevel = 0;
+
    for (std::list<Parameter>::const_iterator i = params_.begin(); i != params_.end(); ++i) {


If you rework this anyway you can turn this into a range base do loop as well

Ozaq · 2025-05-21T08:26:29Z

src/metkit/mars/MarsRequest.cc

@@ -326,8 +327,47 @@ void MarsRequest::getParams(std::vector<std::string>& p) const {

 size_t MarsRequest::count() const {
    size_t result = 1;
+
+    std::set<std::string> paramidsSingleLevel = ParamID::getMlParamsSingleLevel();


this should be const and a reference.

Ozaq · 2025-05-21T08:27:43Z

src/metkit/mars/MarsRequest.cc

+                    if (paramidsSingleLevel.find(v) != paramidsSingleLevel.end()) {
+                        paramsSingleLevel++;


You can use paramsSingleLevel += parmaIdsSingleLevel.count(v);

Ozaq · 2025-05-21T08:28:39Z

src/metkit/mars/MarsRequest.cc

+        }
+        else {
+            if (i->name() == "param") {
+                for (auto v : i->values()) {


const auto&

danovaro added 24 commits April 21, 2025 23:09

improved request expansion tests

deb6df7

MARSC-246 fixes to the language + improved unit tests

8cebb7d

Merge branch 'develop' into feature/inheritance

8ee021f

MARSC-208 added unit tests + facility to add tests with input request…

3a8d716

… + expected result

testing expansions using external files - print offending file in cas…

7fc5eb4

…e of errors

Merge branch 'develop' into feature/inheritance

d232c8e

Merge branch 'develop' into feature/inheritance

68061d7

improved date handling

cb9d4b4

missing include + format

c50c913

missing include + format

82510e3

request count + additional tests + lamnguage format (enum values)

7c2f78d

improved parsing of expected expansion

b5f269f

Merge branch 'develop' into feature/inheritance

82d09e4

wip

0d0c6d8

added support for sub-type definition to handle language extensions (…

5389f3d

…i.e. tigge)

fix unit test

33b3dc3

verb aliases

a6d102e

format

0b195ef

wip

81aad58

bcmodel

6654906

fix Type enum case matching

a7554df

format

ef73189

inheritance at MarsRequest parsing time

078830f

parameter ordering in MarsRequest parsing

28e8e86

danovaro requested review from tbkr, ChrisspyB, Ozaq and simondsmart May 14, 2025 13:06

aliases

9788f42

danovaro added 5 commits May 15, 2025 15:49

Merge branch 'develop' into feature/inheritance

7f7c938

format

5a4fc0e

Merge branch 'develop' into feature/inheritance

1361547

inheritance disseminate (frequency=off)

df3d2f3

clean inheritance on OFF

3b7f22f

Ozaq reviewed May 20, 2025

View reviewed changes

Ozaq reviewed May 21, 2025

View reviewed changes

tests/test_context.cc Outdated

@@ -0,0 +1,57 @@

/*

* (C) Copyright 1996- ECMWF.

Copy link

Contributor

Ozaq May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be 2025

Ozaq reviewed May 21, 2025

View reviewed changes

src/metkit/mars/MarsRequest.cc Outdated

}

else {

if (i->name() == "param") {

for (auto v : i->values()) {

Copy link

Contributor

Ozaq May 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const auto&

addressed PR comments

11d1550

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/inheritance #85

Feature/inheritance #85

danovaro commented May 14, 2025 •

edited

Loading

codecov-commenter commented May 14, 2025 •

edited

Loading

Ozaq May 20, 2025

Ozaq May 20, 2025

Ozaq May 20, 2025

Ozaq May 20, 2025

Ozaq May 20, 2025

Ozaq May 20, 2025

Ozaq May 20, 2025

Ozaq May 20, 2025

Ozaq May 20, 2025

Ozaq May 21, 2025

Ozaq May 21, 2025

Ozaq May 21, 2025

Ozaq May 21, 2025

Ozaq May 21, 2025

danovaro May 21, 2025

Ozaq May 21, 2025

Ozaq May 21, 2025

danovaro May 21, 2025

Ozaq May 21, 2025

Ozaq May 21, 2025

Ozaq May 21, 2025

Ozaq May 21, 2025


		//----------------------------------------------------------------------------------------------------------------------

		ContextRule::ContextRule(const std::string& k) : key_(k) {}


		//----------------------------------------------------------------------------------------------------------------------

		class Include : public ContextRule {

		if (paramidsSingleLevel.find(v) != paramidsSingleLevel.end()) {
		paramsSingleLevel++;

Feature/inheritance #85

Are you sure you want to change the base?

Feature/inheritance #85

Conversation

danovaro commented May 14, 2025 • edited Loading

codecov-commenter commented May 14, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danovaro commented May 14, 2025 •

edited

Loading

codecov-commenter commented May 14, 2025 •

edited

Loading