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 2429 tc pairs members #2451

Merged
merged 14 commits into from
Feb 23, 2023
Merged

Conversation

sethlinden
Copy link
Contributor

Expected Differences

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

    If yes, please describe:

Yes, I added a new configuration field for the consensus[] dictionary. Added write_members field. If this is set to FALSE, output for the consensus members will no longer show up in the output file.

  • 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 a tc_pairs command line passed on the CONSENSUS unit test:

./tc_pairs -adeck /d1/projects/MET/MET_test_data/unit_test/tc_data/adeck/aal132020.dat -bdeck /d1/projects/MET/MET_test_data/unit_test/tc_data/bdeck/bal132020.dat -config ./TCPairsConfig_CONSENSUS -out al132020_CONSENSUS -v 2

After updating ../MET/internal/test_unit/config/TCPairsConfig_CONSENSUS

I also ran the unit test:

unit_tc_pairs.xml

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

Run the unit test:

unit_tc_pairs.xml

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

Yes, I updated the documentation for tc_pairs:

MET/docs/Users_Guide/

tc-pairs.rst

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

No new unit tests were created. Just an updated to the config file: TCPairsConfig_CONSENSUS

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

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

  • Please complete this pull request review by [Fill in date].

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)
    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 Development issue 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.

Seth Linden added 8 commits February 14, 2023 14:25
…then check those boolean values to add to SkipConsensusMembers. SL. ci-skip-all
…string array SkipConsensusMembers to keep track of members to skip output for. Modified filter_tracks to skip the tracks (consensus members) that are listed in SkipConsensusMembers. SL
Making the order of the log messages match the order in which the filtering logic is applied. Putting "Rejected for skip members" first since that's checked first.
Very minor change. Just calling adding all model names in one call rather than looping over them element by element.
Fixing mis-spellings and tweaking wording.
Copy link
Collaborator

@JohnHalleyGotway JohnHalleyGotway left a comment

Choose a reason for hiding this comment

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

I approve of these changes.

Note that consensus.write_members is NOW a required entry. Adding this new requirement may cause a headache for existing users that already have consensus defined in existing TC-Pairs config files. The good news is that this produces an obvious runtime error:

ERROR  : 
ERROR  : Dictionary::lookup_bool() -> lookup failed for name "write_members"
ERROR  : 

I had considered refining the Dictionary::lookup_bool() logic to print a warning message about it being missing instead, but worry about modifying the dictionary lookup logic.

…, making the specification of consensus.write_members optional. If present in the config file, it'll be used. If not, this warning message is printed:

WARNING:
WARNING: TCPairsConfInfo::process_config() -> "consensus.write_members" is missing. Using default value of true.
WARNING:

We chose to do this because it's going into a minor release (v11.1.0).
We do no want to cause v11.0.0 config files to cause an error when used
in v11.1.0.
@JohnHalleyGotway
Copy link
Collaborator

Proceeding with the merge of this PR. Based on feedback from @j-opatz today, I updated the logic to print a warning message if write_members is missing:

WARNING: 
WARNING: TCPairsConfInfo::process_config() -> "consensus.write_members" is missing. Using default value of true.
WARNING: 

But I confirmed that warning message goes away once you add write_members as a true/false boolean.

@JohnHalleyGotway JohnHalleyGotway merged commit 6163c98 into develop Feb 23, 2023
@JohnHalleyGotway JohnHalleyGotway deleted the feature_2429_tc_pairs_members branch February 23, 2023 21:52
JohnHalleyGotway added a commit that referenced this pull request Feb 24, 2023
Co-authored-by: Seth Linden <[email protected]>
Co-authored-by: John Halley Gotway <[email protected]>
Co-authored-by: johnhg <[email protected]>
Co-authored-by: Lisa Goodrich <[email protected]>
Co-authored-by: jprestop <[email protected]>
Co-authored-by: Howard Soh <[email protected]>
Co-authored-by: MET Tools Test Account <[email protected]>
Co-authored-by: Dave Albo <[email protected]>
Co-authored-by: j-opatz <[email protected]>
Co-authored-by: George McCabe <[email protected]>
Co-authored-by: Julie Prestopnik <[email protected]>
Co-authored-by: Jonathan Vigh <[email protected]>
Co-authored-by: Julie Prestopnik <[email protected]>
Co-authored-by: Seth Linden <[email protected]>
Co-authored-by: hsoh-u <[email protected]>
Co-authored-by: lisagoodrich <[email protected]>
Co-authored-by: davidalbo <[email protected]>
Co-authored-by: Daniel Adriaansen <[email protected]>
fix 2238 link error (#2255)
fix #2271 develop nbrctc (#2272)
fix #2309 develop tcmpr (#2310)
fix #2306 ascii2nc airnow hourly (#2314)
fix_spread_md (#2335)
fix #2389 develop flowchart (#2392)
Fix Python environment issue (#2407)
fix definitions of G172 and G220 based on comments in NOAA-EMC/NCEPLIBS-w3emc#157. (#2406)
fix #2380 develop override (#2382)
fix #2408 develop empty config (#2410)
fix #2390 develop compile zlib (#2404)
fix #2412 develop climo (#2422)
fix #2437 develop convert (#2439)
fix for develop, for #2437, forgot one reference to the search_parent for a dictionary lookup.
fix #2452 develop airnow (#2454)
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 TC-Pairs to disable the output of consensus track members
2 participants