Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature #2924 fcst climo, PR 2 of 3 #2942

Merged
merged 67 commits into from
Aug 21, 2024

Conversation

JohnHalleyGotway
Copy link
Collaborator

@JohnHalleyGotway JohnHalleyGotway commented Aug 1, 2024

Expected Differences

MET #2924 is a large development task that has been split into 2 pull requests. PR #2939 contained this first set of changes and this PR contains the second set of changes.

This second pull request includes changes to 25 files, outlined below:

  • [2] Header data files (met_header_columns_V12.0.txt and met_12.0.hdr) renames existing and adds new columns to the MPR and ORANK line types.

  • [3] stat_column_defs.h, stat_columns.cc, and pair_data_point.cc add/modify MPR/ORANK column names.

  • [2] gsi_tools are updated to handle the MPR and ORANK line type changes.

  • [3] docs/Users_Guide files update the documentation and make more consistent use of the .. note:: option (unrelated to this issue). config_options.rst adds the example config setting, as requested.

  • [7] Unit test files. unit_climatology_mixed.xml defines 1 new calls to Grid-Stat with mixed fcst/obs climo data. Column names updated in unit_climatology_1.5deg.xml. 1 new and 2 updated config files in internal/test_unit/config. unit_test.sh and testing.yml now call the new xml file.

  • [1] Minor changes to comments in threshold.cc

  • [7] read_climo.h/.cc are updated to print more descriptive log messages about the data being read, and ensemble_stat.cc, grid_stat.cc, point_stat.cc, series_analysis.cc, and gen_ens_prod.cc are updated to pass those description strings.

  • 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)? [Yes]

    If yes, please describe:

    • In MPR line type:
      • Renames columns CLIMO_MEAN, CLIMO_STDEV, and CLIMO_CDF as OBS_CLIMO_MEAN, OBS_CLIMO_STDEV, and OBS_CLIMO_CDF, respectively.
      • Adds new columns FCST_CLIMO_MEAN and FCST_CLIMO_STDEV.
    • In ORANK line type:
      • Renames columns CLIMO_MEAN, CLIMO_STDEV as OBS_CLIMO_MEAN, OBS_CLIMO_STDEV, respectively.
      • Adds new columns FCST_CLIMO_MEAN and FCST_CLIMO_STDEV.
    • Makes similar name changes in gridded NetCDF output files from Grid-Stat.

Pull Request Testing

  • Describe testing already performed for these changes:

    Tested by adding new unit test. After GHA testing workflow is done, I'll add a description of the diffs ehre.

  • Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:

    Review code changes and documentation updates. Recommend testing with data provided by NOAA.

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

  • Please review the updates docs for this PR: https://met--2942.org.readthedocs.build/en/2942/

  • Do these changes include sufficient testing updates? [Yes]
    Adds 1 new unit test in a new unit_climatology_mixed.xml file.

  • 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:

  • All the MPR and ORANK output is modified.

  • Expect updated NetCDF variable names in the Grid-Stat matched pairs files.

  • Adds new output for new unit test.

  • Will this PR result in changes to existing METplus Use Cases? [Yes]

    If yes, create a new Update Truth METplus issue to describe them.

  • Changes to the MPR and ORANK line types.

  • See Update Truth: For dtcenter/MET#2942 METplus#2656

  • Do these changes introduce new SonarQube findings? [Yes]

    If yes, please describe:

  • In this SonarQube scan flags 1 code smell about a function with > 7 args. This is an existing smell and we actually have many instances of it. We have not decided to tackle this one yet.

  • It also flags 10% duplication, and we've also decided not to attack this yet either.

  • Please complete this pull request review by [Monday 8/5/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.

…uplicate existing climo values, update the header tables and MPR/ORANK documentation tables.
…climo data... but more work to come. Committing this first set of changes that are incomplete but do compile.
…lumn names are requested, the new ones are used.
…ut column. This compiles but not sure if it actually runs yet
…ClimoGrid::init_from_scratch() member function. The constructor had been calling clear() to delete pointers that weren't properly initialized to nullptr. Also, simplify some map processing logic.
…nd to avoid conflicts in member function implementations.
@JohnHalleyGotway
Copy link
Collaborator Author

@j-opatz please find this code compiled on seneca for further testing in /d1/projects/MET/MET_pull_requests/met-12.0.0/beta6/MET-feature_2924_fcst_climo_pr2.

For future reference, here are the commands I used to compile there:

runas met_test
cd /d1/projects/MET/MET_pull_requests/met-12.0.0/beta6
git clone https://github.com/dtcenter/MET MET-feature_2924_fcst_climo_pr2
cd MET-feature_2924_fcst_climo_pr2
git checkout feature_2924_fcst_climo_pr2
source internal/scripts/environment/development.seneca
./configure --prefix=`pwd` --enable-all
make install test >& make.log&
tail -f make.log

@JohnHalleyGotway JohnHalleyGotway changed the title Feature #2924 fcst climo, PR 2 of 2 Feature #2924 fcst climo, PR 2 of 3 Aug 8, 2024
…ed for the corresponding METplus Use Case. Note that there are 22 other TRI values not currently supported.
Copy link
Contributor

@j-opatz j-opatz left a comment

Choose a reason for hiding this comment

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

With data provided by EMC, this PR was tested with both a forecast and observation climatology. Observing both the netCDF output and CNT line type values, it appears that each field (forecast and observation) are correctly having their independent climatology applied. Values are reasonable and reflect what is expected given the dataset.

The column changes were already checked and confirmed for accuracy.

Although the SonarQube check is currently failing, I believe that is expected(?) and this PR is approved.

@JohnHalleyGotway JohnHalleyGotway merged commit e5ad6b2 into develop Aug 21, 2024
37 of 39 checks passed
@JohnHalleyGotway JohnHalleyGotway deleted the feature_2924_fcst_climo_pr2 branch August 21, 2024 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: 🏁 Done
Development

Successfully merging this pull request may close these issues.

Enhance MET to support separate climatology datasets for both the forecast and observation inputs
2 participants