Feature #2924 parse_config

[JohnHalleyGotway]

Expected Differences

This pull request changes the logic for how the climo_mean and climo_stdev dictionaries are parsed. These dictionaries consist of several entries (file_name, field, regrid, time_interp_method, day_interval, hour_interval). Currently all of these entries must be defined within the same dictionary at the same config file context level.

This pull request modifies the logic so that each entry is parsed separately. So while the field information may be parsed from config file context fcst.climo_mean.field, if fcst.climo_mean.day_interval is not defined, the default value in climo_mean.day_interval will be used instead.

This change simplifies the logic for the METplus wrappers. Instead of needing to override ALL climo settings, it now only needs to override the ones explicitly set in the METplus configuration file. And if left unspecified, the default values defined in the default MET configuration file for each tool will be used instead.

• 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:
  
  Please note that I added a DEBUG(5) log message to report the criteria used when searching for climatology data. I reran the xml/unit_climatology_mixed.xml test several times manually, increasing the verbosity level to 5, writing to a log file, and manually modifying the climo_mean and climo_stdev settings in GridStatConfig_climo_FCST_NCEP_1.0DEG_OBS_WMO_1.5DEG and confirming the expected changes appear in that log message.

• Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:
  
  Please find this code compiled by the met_test user in seneca:/d1/projects/MET/MET_pull_requests/met-12.0.0/beta6/MET-feature_2924_parse_config. Recommend testing in a similar way as described above:

runas met_test
cd /d1/projects/MET/MET_pull_requests/met-12.0.0/beta6/MET-feature_2924_parse_config/internal/test_unit
testenv # to set env vars
./run_unit_climatology_mixed.sh
grep "Searching" run_unit.log
# Manually modify config/GridStatConfig_climo_FCST_NCEP_1.0DEG_OBS_WMO_1.5DEG
# Re-run the script above, grep for "Searching" and confirm the log messages match your config changes

• Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [No]
  None needed.

• Do these changes include sufficient testing updates? [No]
  None needed.

• 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? [TBD]
  
  If yes, please describe:

• Please complete this pull request review by [Monday, 9/9/24].
  

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: Coordinated METplus-X.Y Support project for bugfix releases or MET-X.Y.Z Development project for official releases
• 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.

[JohnHalleyGotway]

Converting this PR to a draft to investigate unexpected differences flagged by the GHA testing workflow.

[j-opatz]

I've reviewed the changed files, as well as performed the tests indicated in the PR. I confirmed that the various settings of climo_mean and climo_stdev libraries are now pulled independently, ignoring the levels at which the other settings appear. This logic should now allow users to take advantage of the new read-in abilities for climatology while removing the requirement to adjust all configuration files to a certain style (i.e. the climo dictionaries are moved to the fcst and obs dictionaries).

[georgemccabe]

I'm getting some errors that I didn't expect when running your tests after modifying the config file. The changes below are in seneca:/d1/projects/MET/MET_pull_requests/met-12.0.0/beta6/MET-feature_2924_parse_config/internal/test_unit/config/GridStatConfig_climo_FCST_NCEP_1.0DEG_OBS_WMO_1.5DEG

I would expect to be able to set fcst.climo_mean.regrid.method without any of the other fcst.climo_mean.regrid values. If I set:

regrid = {
	method = UPPER_LEFT;
      }

I get:

ERROR : Dictionary::lookup_int() -> lookup failed for name "width"

If I add, width:

      regrid = {
	method = UPPER_LEFT;
        width      = 3;
      }

I get:

ERROR : met_regrid() -> bad interpolation method ... UPPER_LEFT

but I see UPPER_LEFT as a valid regrid.method in the documentation.

[JohnHalleyGotway]

Thanks for the feedback @georgemccabe.

I did update the logic in vx_regrid.cc to add support for the 4 corner point regridding methods. Thanks for catching that. The fix is easy.

I do understand why you're getting the other error based on the current set of changes. While I had updated the logic for parsing the climo_mean and climo_stdev dictionaries, I had not done so for the child dictionaries, like regrid. However, I worked on a way to support the behavior you expected, and we'll see how it all works in the tests for this PR.

[JohnHalleyGotway]

@georgemccabe thanks for your review. I've finished addressing the issues you've raised and re-requested your review on this PR. I'd like to point you to:

1. This GHA Testing Workflow ran without error and produced no differences in the results.
2. I recompiled the code with these updates in seneca:/d1/projects/MET/MET_pull_requests/met-12.0.0/beta6/MET-feature_2924_parse_config for your further testing.

[JohnHalleyGotway]

@georgemccabe OK I pushed more changes to this feature_2924_parse_config branch and recompiled on seneca. Please take a look and see if the behavior matches what you expect. Note that I did modify a lot of log messages so that whenever regridding occurs, it now tells you what method is being used, like this:

DEBUG 2: Regridding 19590410_000000 forecast climatology standard deviation field TMP/P500 to the verification grid using UPPER_RIGHT(1).

[georgemccabe]

I reviewed the test log after more changes were made and everything looks good. Thanks for iterating on this!

[JohnHalleyGotway]

Thanks @georgemccabe and @j-opatz for the feedback on this PR. I'll proceed with merging and will monitor the downstream METplus Use Case runs to see if any output unexpectedly changes.