Feature 3254 ugrid at notation by hsoh-u · Pull Request #3368 · dtcenter/MET

hsoh-u · 2026-04-02T20:34:46Z

Expected Differences

Support @notatpn on vertical level

Do these changes introduce new tools, command line arguments, or configuration file options? [No]

If yes, please describe:
Do these changes modify the structure of existing or add new output data types (e.g. statistic line types or NetCDF variables)? [No]

If yes, please describe:

Pull Request Testing

Describe testing already performed for these changes:

command: ./point_stat /d1/projects/MET/MET_test_data/unit_test/ugrid_data/lfric/lfric_2012-04-09_00.00.00.nc pb2nc/gdas1.20120409.t12z.prepbufr.nc PointStatConfig_ugrid_lfric_pressure_levels -ugrid_config UGridConfig_lfric_pressure_levels -outdir point_stat_ugrid -v 4

Changed ./internal/test_unit/config/PointStatConfig_ugrid_lfric_pressure_levels

cst = {
   ...

   field = [
      { name = "u_in_w3"; level = "(21,*)"; set_attr_level = "P500"; },
      { name = "u_in_w3"; level = "(24,*)"; set_attr_level = "P850"; }
   ];
}

to

cst = {
   ...

   field = [
      { name = "u_in_w3"; level = "(@500,*)"; set_attr_level = "P500"; },
      { name = "u_in_w3"; level = "(@850,*)"; set_attr_level = "P850"; }
   ];
}

Tested an error case:
command: ./point_stat /d1/projects/MET/MET_test_data/unit_test/ugrid_data/lfric/lfric_2012-04-09_00.00.00.nc pb2nc/gdas1.20120409.t12z.prepbufr.nc PointStatConfig_ugrid_lfric_pressure_levels -ugrid_config UGridConfig_lfric_pressure_levels -outdir point_stat_ugrid -v 4

cst = {
   ...

   field = [
      { name = "u_in_w3"; level = "(@501,*)"; set_attr_level = "P500"; },
      { name = "u_in_w3"; level = "(@850,*)"; set_attr_level = "P850"; }
   ];
}

==>
ERROR  :
ERROR  : MetUGridDataFile::data_plane() -> the vertical level value (501) does not exist at the pressure variable
ERROR  :

Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:
Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [No]
Do these changes include sufficient testing updates? [No]

The existing unit test was modified to use a @Notation.

Will this PR result in changes to the MET test suite? [No]

If yes, describe the new output and/or changes to the existing output:
Will this PR result in changes to existing METplus Use Cases? [No]

If yes, create a new Update Truth METplus issue to describe them.
Do these changes introduce new SonarQube findings? [No]

If yes, please describe:
Please complete this pull request review by [Fill in date].

Pull Request Checklist

See the METplus Workflow for details.

Review the source issue metadata (required labels, projects, and milestone).
Complete the PR definition above.
Ensure the PR title matches the feature or bugfix branch name.
Define the PR metadata, as permissions allow.
Select: Reviewer(s) and Development issue
Select: Milestone as the version that will include these changes
Select: METplus-X.Y Support project for bugfix releases or MET-X.Y Development project for the next coordinated release
After submitting the PR, select the ⚙️ icon in the Development section of the right hand sidebar. Search for the issue that this PR will close and select it, if it is not already selected.
After the PR is approved, merge your changes. If permissions do not allow this, request that the reviewer do the merge.
Close the linked issue and delete your feature or bugfix branch from GitHub.

…_at_notation

Copilot

Pull request overview

This PR adds support for using @<value> notation for UGrid vertical level selection (value-based lookup) and refactors shared VarInfo “magic string” setup logic across NetCDF/UGRID VarInfo implementations.

Changes:

Added common VarInfo helpers to parse/set name + level from config dictionaries and to centralize shared set_magic logic.
Implemented value-to-vertical-index lookup for UGrid slicing to support (@<level>,*) configurations.
Applied small C++ cleanups (const-correctness, empty() usage, loop modernizations) in UGrid-related code.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/libcode/vx_data2d/var_info.h	Declares new shared helper methods for config parsing and magic string setup.
src/libcode/vx_data2d/var_info.cc	Implements shared parsing and `set_magic_common` helper used by multiple VarInfo subclasses.
src/libcode/vx_data2d_ugrid/var_info_ugrid.cc	Uses shared magic setup and relies on value-based `@` parsing for dimensions.
src/libcode/vx_data2d_ugrid/ugrid_file.h	Adds `z_var_name` and const-ref API adjustments.
src/libcode/vx_data2d_ugrid/ugrid_file.cc	Tracks vertical coordinate variable name; refactors unit conversion checks and config handling.
src/libcode/vx_data2d_ugrid/data2d_ugrid.h	Updates signatures for const-correctness and adds vertical-offset helper declaration.
src/libcode/vx_data2d_ugrid/data2d_ugrid.cc	Implements vertical value-to-index mapping and uses it for `@`-style vertical slicing.
src/libcode/vx_data2d_nc_wrf/var_info_nc_wrf.cc	Switches to shared magic setup and uses parsed `Name`/`Level` from base `set_dict`.
src/libcode/vx_data2d_nc_met/var_info_nc_met.cc	Switches to shared magic setup and uses parsed `Name`/`Level` from base `set_dict`.
src/libcode/vx_data2d_nc_cf/var_info_nc_cf.cc	Switches to shared magic setup and uses parsed `Name`/`Level` from base `set_dict`.
internal/test_unit/config/PointStatConfig_ugrid_lfric_pressure_levels	Updates the test config to use `@` notation (currently only for one of the two levels).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-02T20:38:26Z

src/libcode/vx_data2d_ugrid/ugrid_file.cc

+  if (get_var_units(_latVar, units_value) && units_value == "rad" || units_value == "radian") {
+    mlog << Debug(6) << method_name << "convert  " << units_value << " to degree for lat\n";
+    for (int idx=0; idx<face_count; idx++) _lat[idx] /= rad_per_deg;
  }
-  if (get_var_units(_lonVar, units_value)) {
-    if (units_value == "rad" || units_value == "radian") {
-      mlog << Debug(6) << method_name << "  convert " << units_value << " to degree for lon\n";
-      for (int idx=0; idx<face_count; idx++) _lon[idx] /= rad_per_deg;
-    }
+  if (get_var_units(_lonVar, units_value) && units_value == "rad" || units_value == "radian") {
+    mlog << Debug(6) << method_name << "  convert " << units_value << " to degree for lon\n";
+    for (int idx=0; idx<face_count; idx++) _lon[idx] /= rad_per_deg;


The unit-conversion condition has incorrect operator precedence: get_var_units(...) && units_value == "rad" || units_value == "radian" evaluates the || last, so it can convert even when get_var_units returns false (and/or when units_value is left over from the previous check). Wrap the unit comparison in parentheses so conversion only happens when the units attribute is present and matches (e.g., get_var_units(...) && (units_value == "rad" || units_value == "radian")).

Copilot · 2026-04-02T20:38:26Z

src/libcode/vx_data2d/var_info.cc

+   // Check for embedded whitespace
+   if((unsigned int) nstr.length() != strcspn(nstr.c_str(), " \t") ||
+      (unsigned int) lstr.length() != strcspn(lstr.c_str(), " \t")) {
+      mlog << Error << "\nnetcdf_set_magic_util::set_magic_common() -> "


set_magic_common logs an error prefix of netcdf_set_magic_util::set_magic_common(), but the function is VarInfo::set_magic_common. This makes troubleshooting harder and is inconsistent with VarInfo::set_magic(). Update the logged method name to the correct one.

Suggested change

mlog << Error << "\nnetcdf_set_magic_util::set_magic_common() -> "

mlog << Error << "\nVarInfo::set_magic_common() -> "

Copilot · 2026-04-02T20:38:26Z

src/libcode/vx_data2d_ugrid/data2d_ugrid.cc

+               if (_file->ValidTime[idx] > time_upper
+                   || _file->ValidTime[idx] < next_time) continue;


In the incremented time-range scan, the loop no longer breaks when _file->ValidTime[idx] > time_upper. For monotonically increasing times this causes unnecessary scanning to the end of the array (potentially large) after the first out-of-range value. Consider restoring the early break for the > time_upper case and only continue for values < next_time.

Suggested change

if (_file->ValidTime[idx] > time_upper

|| _file->ValidTime[idx] < next_time) continue;

if (_file->ValidTime[idx] > time_upper) break;

if (_file->ValidTime[idx] < next_time) continue;

internal/test_unit/config/PointStatConfig_ugrid_lfric_pressure_levels

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (2)

src/libcode/vx_data2d_ugrid/data2d_ugrid.cc:679

extract_vlevels() does not modify var_name_base, but it now takes a non-const reference. Making this parameter a const reference would better reflect intent, prevent accidental mutation, and allow passing temporaries without requiring a named variable.

int MetUGridDataFile::extract_vlevels(ConcatString &var_name_base, const char *var_name) const {
   int num_mat = 0;
   char** mat = nullptr;
   int vlevel = bad_data_int;
   ConcatString name_pattern;

   name_pattern.erase();
   name_pattern << "(" << var_name_base << "[ _])([0-9]+)(.*)";
   num_mat = regex_apply(name_pattern.c_str(), 4, var_name, mat);

src/libcode/vx_data2d_ugrid/ugrid_file.cc:810

get_metadata_names() returns metadata_map[key] after a successful find(). Using the iterator’s value (search->second) avoids a second map lookup and avoids operator[] (which can insert on misses) if this code is refactored later.

StringArray UGridFile::get_metadata_names(const std::string &key) {
  StringArray empty;
  auto search = metadata_map.find(key);
  return search == metadata_map.end() ? empty : metadata_map[key];
}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-03T17:50:51Z

src/libcode/vx_data2d/var_info.cc

+void VarInfo::parse_and_set_name_level(Dictionary &dict) {
+   ConcatString nstr = dict.lookup_string(conf_key_name, false);
+   ConcatString lstr = dict.lookup_string(conf_key_level, false);
+   if(nstr.nonempty()) {
+      set_name(nstr);
+      set_req_name(nstr.c_str());
+   }


parse_and_set_name_level() uses dict.lookup_string(conf_key_name, false), which makes the field "name" optional. Several derived VarInfo::*::set_dict() implementations previously used dict.lookup_string("name") (error_out default true), so a missing name would fail fast; now it can silently leave Name/ReqName unset and lead to later lookup failures or confusing behavior. Consider making "name" required again (and honoring do_exit), e.g., look it up with error_out tied to do_exit and/or explicitly call handle_config_error when it’s missing.

Copilot · 2026-04-03T17:50:51Z

src/libcode/vx_data2d/var_info.cc

+   // Check for embedded whitespace
+   if((unsigned int) nstr.length() != strcspn(nstr.c_str(), " \t") ||
+      (unsigned int) lstr.length() != strcspn(lstr.c_str(), " \t")) {
+      mlog << Error << "\nVarInfo::set_magic_common() -> "
+           << "embedded whitespace found in the name \"" << nstr
+           << "\" or level \"" << lstr << "\" string.\n\n";
+      exit(1);
+   }
+   // Format as {name}/{level} or {name}{level}
+   if(lstr.nonempty() && lstr[0] != '(') {
+      MagicStr << cs_erase << nstr << "/" << lstr;
+   }
+   else {
+      MagicStr << cs_erase << nstr << lstr;
+   }


set_magic_common() duplicates the whitespace check and magic-string formatting logic already present in VarInfo::set_magic(). To reduce drift risk, consider calling VarInfo::set_magic(nstr, lstr) and then doing the shared name/req_name updates (or refactor so the common formatting is implemented once).

Suggested change

// Check for embedded whitespace

if((unsigned int) nstr.length() != strcspn(nstr.c_str(), " \t") ||

(unsigned int) lstr.length() != strcspn(lstr.c_str(), " \t")) {

mlog << Error << "\nVarInfo::set_magic_common() -> "

<< "embedded whitespace found in the name \"" << nstr

<< "\" or level \"" << lstr << "\" string.\n\n";

exit(1);

}

// Format as {name}/{level} or {name}{level}

if(lstr.nonempty() && lstr[0] != '(') {

MagicStr << cs_erase << nstr << "/" << lstr;

}

else {

MagicStr << cs_erase << nstr << lstr;

}

set_magic(nstr, lstr);

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-03T18:32:27Z

src/libcode/vx_data2d_ugrid/ugrid_file.cc

  _latVar = (NcVar *)nullptr;
  _lonVar = (NcVar *)nullptr;
+  z_var_name.clear();
  metadata_map.clear();
  max_distance_km = bad_data_double;


z_var_name is cleared in init_from_scratch(), but close() doesn’t reset it. Since open() calls close() when reusing a UGridFile instance, z_var_name can carry over from a previous file and produce misleading diagnostics. Consider clearing z_var_name in close() as well.

Copilot · 2026-04-03T18:32:27Z

src/libcode/vx_data2d_ugrid/data2d_ugrid.cc

+   static const string method_name_s
+         = "MetUGridDataFile::data_plane() -> ";
   static const string method_name
         = "MetUGridDataFile::data_plane(VarInfo &, DataPlane &, NcVarInfo *) -> ";


The method_name string still references the old signature (NcVarInfo *) even though the function now takes const NcVarInfo *. Updating the string will avoid confusion when debugging logs.

Suggested change

= "MetUGridDataFile::data_plane(VarInfo &, DataPlane &, NcVarInfo *) -> ";

= "MetUGridDataFile::data_plane(VarInfo &, DataPlane &, const NcVarInfo *) -> ";

Copilot · 2026-04-03T18:32:28Z

src/libcode/vx_data2d_ugrid/data2d_ugrid.cc

+int MetUGridDataFile::extract_vlevels(ConcatString &var_name_base, const char *var_name) const {
   int num_mat = 0;
   char** mat = nullptr;
   int vlevel = bad_data_int;


extract_vlevels() uses regex_apply(...) which allocates match buffers that must be released with regex_clean(mat). The current implementation does not clean up mat, causing a leak each time this helper is called (often inside loops while scanning candidate variable names).

sonarqubecloud · 2026-04-07T20:40:43Z

Quality Gate failed

Failed conditions
4.4% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

U-CIT\hsoh and others added 7 commits March 20, 2026 12:17

#3254 Separated the duplicated codes to parse_and_set_name_level

efd765e

#3254 Added set_magic_common

a77db84

#3254 Added z_var_name

d9fd17a

#3254 Added get_vertical_offset

d6f76c3

Merge remote-tracking branch 'origin/develop' into feature_3254_ugrid…

600ba42

…_at_notation

#3254 Cleanup to reduce SonarQube findings

55f9062

#3254 Changed pressure offset 24 to value 850

11e032f

hsoh-u added this to the MET-13.0.0 milestone Apr 2, 2026

hsoh-u requested review from JohnHalleyGotway and Copilot April 2, 2026 20:34

hsoh-u added this to METplus-13.0 Development Apr 2, 2026

github-project-automation bot moved this to 🩺 Needs Triage in METplus-13.0 Development Apr 2, 2026

Copilot started reviewing on behalf of hsoh-u April 2, 2026 20:35 View session

Copilot AI reviewed Apr 2, 2026

View reviewed changes

Howard Soh added 3 commits April 3, 2026 17:42

#3254 Correct the method_name of set_magic_common

c7e3c76

3254 Corrected merge error with if

8ed1232

#3254 Fixed SonarQube findings

ff52a8b

hsoh-u requested a review from Copilot April 3, 2026 17:44

Copilot started reviewing on behalf of hsoh-u April 3, 2026 17:45 View session

Copilot AI reviewed Apr 3, 2026

View reviewed changes

#3254 Reduced the duplicated codes

24ab821

hsoh-u requested a review from Copilot April 3, 2026 18:26

Copilot started reviewing on behalf of hsoh-u April 3, 2026 18:27 View session

Copilot AI reviewed Apr 3, 2026

View reviewed changes

U-CIT\hsoh and others added 3 commits April 3, 2026 15:17

#3254 Reduced the duplicated codes

1512916

#3254 Added cleanup

8cfa20f

#3254 Removed duplicated cleanup()

10250ed

JohnHalleyGotway moved this from 🩺 Needs Triage to 🔎 In review in METplus-13.0 Development Apr 7, 2026

#3254 Removed cleanup

0692909

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature 3254 ugrid at notation#3368

Feature 3254 ugrid at notation#3368
hsoh-u wants to merge 15 commits intodevelopfrom
feature_3254_ugrid_at_notation

hsoh-u commented Apr 2, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 2, 2026

Uh oh!

Copilot AI Apr 2, 2026

Uh oh!

Copilot AI Apr 2, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 3, 2026

Uh oh!

Copilot AI Apr 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 3, 2026

Uh oh!

Copilot AI Apr 3, 2026

Uh oh!

Copilot AI Apr 3, 2026

Uh oh!

sonarqubecloud bot commented Apr 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	mlog << Error << "\nnetcdf_set_magic_util::set_magic_common() -> "
	mlog << Error << "\nVarInfo::set_magic_common() -> "

		if (_file->ValidTime[idx] > time_upper
		\|\| _file->ValidTime[idx] < next_time) continue;

	= "MetUGridDataFile::data_plane(VarInfo &, DataPlane &, NcVarInfo *) -> ";
	= "MetUGridDataFile::data_plane(VarInfo &, DataPlane &, const NcVarInfo *) -> ";

Conversation

hsoh-u commented Apr 2, 2026

Expected Differences

Pull Request Testing

Pull Request Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Apr 7, 2026

Quality Gate failed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants