Feature #2924 parse_config PR 2 #2975
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…eparately from each obs.field entry because the obs dictionary does not exist in the GenEnsProd config file.
…can perform lookups for the parent.
…s that retrieve the climatology data. Rather than requiring all the climo_mean and climo_stdev dictionary entries to be defined at the same config file context level, parse each one individually. This enables the METplus wrappers to only partially override this dictionary and still rely on the default values provided in MET's default configuration files.
…d on the new function signature. Also update the tools to check the number of climo fields separately for the forecast and observation climos.
…nary. Use config.fcst.climo_mean.regrid first, config.fcst.regrid second, and config.climo_mean.regrid third. Notably, DO NOT use config.regrid. This is definitely the problem with having regrid specified at mutliple config file context levels. It makes the logic for which to use when very messy.
… the climatology regridding logic inside fcst is problematic because it applies to the forecast data as well and you end up with the verification grid being undefined. So the climo regridding logic must be defined in 'climo_mean.regrid' either within the 'fcst' and 'obs' dictionaries or at the top-level config context.
…t, Upper_Right, Lower_Right, and Lower_Left interpolation methods to the list of valid options for regridding, as already indicated in the MET User's Guide.
…e it work the way @georgemccabe expects it to. It now uses pointers to both the primary and default dictionaries and parses each entry individually.
… the regrid dictionary.
…ing' log message to avoid confusion.
…umber of climo fields provided. If there's 0, just return since no data has been requested. If there's 1, use it regardless of the number of input fields. If there's more than 1, just use the requested i_vx index value.
…ooping over the series entries.
28 tasks
…strings for consistency.
…is PR but works after the changes in this PR.
georgemccabe
approved these changes
Sep 19, 2024
|
Thanks @georgemccabe. Yes, the SonarQube failure is fine. It just flags a function as being too complex (by a longshot!) but refactoring that is beyond the scope of this work. Merging now... |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Expected Differences
This PR adds a new test named
climatology_SERIES_ANALYSIS_1.0DEG_CONST_CLIMOthat ERRORS OUT prior to this PR but works fine with these changes. Note that this PR addresses this METplus Use Case failure:Use Case Tests (s2s:4)
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:
Ran the failing METplus use case before/after these changes to make sure it now works.
Same with the new MET Unit Test I've added.
Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:
Review code changes and check the status of the GHA tests.
Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [Yes]
None needed.
Do these changes include sufficient testing updates? [Yes]
Yes, the new unit test
climatology_SERIES_ANALYSIS_1.0DEG_CONST_CLIMOworks fine on this feature branch but errors out with the current version in the develop branch:Note that the new unit test does write the ANOM_CORR statistic to demonstrate that climo data really is being read.
Will this PR result in changes to the MET test suite? [Yes]
If yes, describe the new output and/or changes to the existing output:
1 new series_analysis output file.
Will this PR result in changes to existing METplus Use Cases? [No]
If yes, create a new Update Truth METplus issue to describe them.
However the failing METplus Use Case should no longer fail.
Do these changes introduce new SonarQube findings? [Maybe]
If yes, please describe:
It will probably flag the complexity of the existing code that I touched. I recommend not fixing any of this for this PR.
Please complete this pull request review by [Fri 9/20/24].
Pull Request Checklist
See the METplus Workflow for details.
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