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 1583 es hira #2056

Merged
merged 29 commits into from
Feb 18, 2022
Merged

Feature 1583 es hira #2056

merged 29 commits into from
Feb 18, 2022

Conversation

JohnHalleyGotway
Copy link
Collaborator

@JohnHalleyGotway JohnHalleyGotway commented Feb 17, 2022

Expected Differences

Whew, well this was a long time in coming, but should now be working as expected.

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

    If yes, please describe:

    In Ensemble-Stat, add support for interp.method = HIRA. For example:
interp = {
  type = [
    { method = NEAREST; width = 1; },
    { method = HIRA; width = 2; },
    { method = HIRA; width = 5; }
  ];
}
  • 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:

    While this does not change the output line types, I want to point out 2 details:
  1. The Ensemble-Stat output can now contain INTERP_MTHD = HIRA, whereas it did not previously.
  2. Previously, the ensemble size remained constant for each run of the tool. With HiRA, that is no longer the case since the number of members varies based on the size of the neighborhood.

The length of the ORANK, RHIST, and RELP output line types vary based on the number of members. Running HiRA messes up the alignment of the output _orank.txt, _rhist.txt, and _relp.txt output files. In this version, the header columns are written based on the MAXIMUM number of ensemble members.

If preferred, I could modify the logic to NOT WRITE any header columns past the LINE_TYPE column when HiRA is used. So please let me know if that's preferable.

Pull Request Testing

  • Describe testing already performed for these changes:

    Tested locally with a variety of HiRA settings to confirm it works as expected. Used GHA to confirm that these changes don't change the existing unit test output... until I modified the configuration for one.

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

  • @mpm-meto please review the doc updates and feel free to make recommended edits.

  • @j-opatz please review the doc updates, config file changes, and see the impact of using HiRA in the output of GHA tests.

  • @jprestop please review the actual code changes. Is the small test I added sufficient?

  • Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [Yes]
    Updated the Ensemble-Stat and config file chapters.

  • Do these changes include sufficient testing updates? [Yes]
    I just tweaked an existing EnsembleStatConfig file that used when you run "make test". It's also used by unit_met_test_scripts.xml. Also tweaked the Stat-Analysis job that processes the Ensemble-Stat output.
    I considered modifying the configuration used in unit_ensemble_stat.xml but realized it would impact lots and lots of output files. I wanted to avoid a very large impact in the results.

  • Will this PR result in changes to the test suite? [Yes]

    If yes, describe the new output and/or changes to the existing output:

    Adds HIRA output lines to the output in met_test_scripts/ensemble_stat and modifies the output of met_test_scripts/stat_analysis.

  • Please complete this pull request review by [Fri 2/18/22].

Pull Request Checklist

See the METplus Workflow for details.

  • 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)
    Select: Organization level software support Project or Repository level development cycle Project
    Select: Milestone as the version that will include these changes
  • After submitting the PR, select Linked issues with the original issue number.
  • 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. Still need to update the application code to use it.
…on methods plus the number of HiRA widths.
… of columns based on the maximum HiRA ensemble size.
… dictionary rather than ens.field, which fails when ens.field is an empty array.
…nsemble members when creating the output txt files... esp when HiRA is not selected. ci-run-unit
…HiRA as an interpolation method for Ensemble-Stat. Having the entire HiRA dictionary in Ensemble-Stat brings unecessary complications when the only part that we actually want/need is the size and shape. This simpler solution requires less documentation updates and should have not upstream impacts on METplus.
…t. This is also run by unit_met_test_scripts.xml. Also, update the downstream STAT-Analysis job to produce output -by INTERP_MTHD,INTERP_PNTS.
@JohnHalleyGotway JohnHalleyGotway linked an issue Feb 17, 2022 that may be closed by this pull request
22 tasks
Copy link
Collaborator

@jprestop jprestop left a comment

Choose a reason for hiding this comment

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

I reviewed the code changes. I think the small test you added is sufficient. I think it's good that the EnsembleStatConfig file you modified runs with "make test" and that it is used by used by unit_met_test_scripts.xml and agree that if modifying the configuration used in unit_ensemble_stat.xml would impact lots and lots of output files, we should avoid that. I approve this request.

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.

The updated documentation looks appropriate for the changes made, and the errors in automated testing is from the expected difference in outputs. It seems like the changes made address the need for HIRA in ensembles, although I'm not too familiar with the process. From what I was reviewing for, I approve this PR.

@JohnHalleyGotway JohnHalleyGotway removed the request for review from mpm-meto February 18, 2022 23:15
@JohnHalleyGotway JohnHalleyGotway merged commit a2a7372 into develop Feb 18, 2022
@JohnHalleyGotway JohnHalleyGotway deleted the feature_1583_es_hira branch February 18, 2022 23:33
JohnHalleyGotway added a commit that referenced this pull request Feb 20, 2022
Co-authored-by: Julie Prestopnik <jpresto@seneca.rap.ucar.edu>
Co-authored-by: johnhg <johnhg@ucar.edu>
Co-authored-by: Seth Linden <linden@kiowa.rap.ucar.edu>
Co-authored-by: John Halley Gotway <johnhg@ucar.edu>
Co-authored-by: j-opatz <59586397+j-opatz@users.noreply.github.com>
Co-authored-by: Howard Soh <hsoh@kiowa.rap.ucar.edu>
Co-authored-by: John Halley Gotway <johnhg@kiowa.rap.ucar.edu>
Co-authored-by: jprestop <jpresto@ucar.edu>
Co-authored-by: Howard Soh <hsoh@seneca.rap.ucar.edu>
Co-authored-by: Seth Linden <linden@ucar.edu>
Co-authored-by: hsoh-u <hsoh@ucar.edu>
Co-authored-by: George McCabe <23407799+georgemccabe@users.noreply.github.com>
Co-authored-by: John Halley Gotway <johnhg@seneca.rap.ucar.edu>
Co-authored-by: MET Tools Test Account <met_test@seneca.rap.ucar.edu>
Co-authored-by: mo-mglover <78152252+mo-mglover@users.noreply.github.com>
Co-authored-by: davidalbo <dave@ucar.edu>
Co-authored-by: lisagoodrich <33230218+lisagoodrich@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance Ensemble-Stat to apply the HiRA method to ensembles
3 participants