-
Notifications
You must be signed in to change notification settings - Fork 24
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 1904 gen_ens_prod #1927
Conversation
…f the existing Ensemble-Stat tool, just renamed to Gen-Ens-Prod, including the newly added documentation section.
…S_ to _GEP_ to get rid of RTD documentation warnings.
…d add the default configuration file.
… to Gen-Ens-Prod.
…omplete. Continue edits in the configuration section.
…data for use in defining climo cdp threshold types.
…() followed by add_const(double, int). That saves code in gen_ens_prod.
…the unit tests. Still need more work to get climo working in unit tests.
…mean/stdev for CDP type thresholds.
…climo data to the fractional_coverage() function.
…ge() function. Also update the gen_ens_prod unit tests to call it once with/without a control member. I manually confirmed that the spread is the same in the output but the means differ.
…move unused rng and tmp_dir config entries. Simplify variable names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good to me. It looks like the new wrapper will be able to borrow existing components from other wrappers to implement all of the functionality. The wrapper will support the following functionality:
- Obtain a list of input files, write a file list file, and pass to -ens argument
- Set environment variables to override values in MET config file (all dictionaries and variables in the new config file are supported in other wrappers)
- Pass wrapped config file as -config argument
- Build output file path from template/dir and pass to -out argument
- Set optional -ctrl path based on template/dir (GEN_ENS_PROD_CTRL_[TEMPLATE/DIR] or GEN_ENS_PROD_CONTROL_[TEMPLATE/DIR])
While I approve these changes, the science review should be completed before merging this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed that the output from the "make test" matched expectations, and reviewed documentation for accuracy and completeness. Also was able to successfully run CPC's NMME data through the new tool, which was previously accomplished in EnsembleStat. Comparisons to the output netCDF files show an exact match for the probability fields.
However, I don't know if the -ctrl option is working as expected. During testing, I ran two instances: one where gen-ens-prod had no control members, but 0-23 ensemble members included (24 total members), and the second where 1 ensemble member was excluded from the list (so 23 total members), but included the missing member as the control member. If this method was functioning correctly, then the ensemble_flag "mean" field should be identical between the two runs, and the "stdev" field should differ.
However, both fields differed from each other. Everything else on the tool seemed to function correctly.
…emble sum is used to compute both the mean and the standard deviation. Since we include control member in the mean but not the standard deviation, we need to track two different versions of that sum.
@j-opatz you're right. Thanks for catching this. There was an error in the logic for tracking the counts. I just committed a fix for it. I also updated the version on kiowa and recompiled: Please rerun your test. Note, I did already confirm the problem/fix in my own testing. FYI, at 4:00pm on 10/1/21, I restarted the full regression test vs develop after merging recent enhancements from develop into this feature branch. That required some manual conflict resolution: |
…ude support was added to develop. Had to manually resolve some conflicts.
… fractional_coverage() utility functions as their signatures where changed by recent enhancements in develop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reran the test for -ctrl option and confirmed that the output was as expected. With this update, I approve the PR merge
Co-authored-by: Seth Linden <linden@kiowa.rap.ucar.edu> Co-authored-by: John Halley Gotway <johnhg@ucar.edu> Co-authored-by: Howard Soh <hsoh@kiowa.rap.ucar.edu> Co-authored-by: johnhg <johnhg@ucar.edu> Co-authored-by: Julie.Prestopnik <jpresto@ucar.edu> Co-authored-by: ericgilleland <34106925+ericgilleland@users.noreply.github.com> Co-authored-by: George McCabe <23407799+georgemccabe@users.noreply.github.com> Co-authored-by: John Halley Gotway <johnhg@kiowa.rap.ucar.edu> Co-authored-by: jprestop <jpresto@ucar.edu> Co-authored-by: Julie Prestopnik <jpresto@seneca.rap.ucar.edu> Co-authored-by: hsoh-u <hsoh@ucar.edu> Co-authored-by: Keith Searight <keith.searight@noaa.gov> Co-authored-by: Seth Linden <linden@ucar.edu> Co-authored-by: MET Tools Test Account <met_test@seneca.rap.ucar.edu> Co-authored-by: lisagoodrich <33230218+lisagoodrich@users.noreply.github.com> Co-authored-by: MET Tools Test Account <met_test@kiowa.rap.ucar.edu> Co-authored-by: j-opatz <59586397+j-opatz@users.noreply.github.com>
Pull Request Testing
This PR adds a new gen_ens_prod tool. It processes the "ens" dictionary that is also currently processed by the Ensemble-Stat tool. However gen_ens_prod add support for climo_mean/stdev to define climatological percentile thresholds. Note that this PR is for an initial version of the tool. Additional enhancements will be made via other issues/PR's.
Describe testing already performed for these changes:
Manually tested. Added call to gen_ens_prod in "make test" and also added that call to unit_met_test_scripts.xml. Also added unit_gen_ens_prod.xml to run it with climo_mean/stdev both with and without the -ctrl command line option.
Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:
@georgemccabe, please review the usage statement, docs, and config file options to understand the functionality of this tool. We need to create a METplus wrapper for it.
@j-opatz, please review in more depth:
Inspect the usage statement, review the config file options, see the output of running "make test", and the output of running "unit_gen_ens_prod.xml". Confirm that the handling of the "-ctrl" member is correct. Its included in the mean and probabilities but excluded from the standard deviation. Please do additional testing to try to break it. Confirm that it can re-create the CPC tercile probability values. Please consider other functionality that should be included in this initial version. The "-out" option requires that the user choose an output file name (like gen_vx_mask does) instead of automatically picking one for them.
Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [Yes]
Added a new chapter that can be reviewed in Read-The-Docs.
Do these changes include sufficient testing updates? [Yes]
See above. Adds call in make test, unit_met_test_scripts.xml, and unit_gen_ens_prod.xml.
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 3 new gen_ens_prod output files:
Pull Request Checklist
See the METplus Workflow for details.
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