Feature 2476 tc pairs diag #2694

sethlinden · 2023-09-27T21:13:55Z

Expected Differences

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:

The changes add consensus diagnostic values to the consensus output file

Pull Request Testing

Describe testing already performed for these changes:

I used the new tc_pairs unit test: tc_pairs_DIAGNOSTICS_CONSENSUS to formulate my own command line:

./tc_pairs -adeck /d1/projects/MET/MET_test_data/unit_test/tc_data/adeck/aal092022.dat -bdeck /d1/projects/MET/MET_test_data/unit_test/tc_data/bdeck/bal092022.dat -diag CIRA_DIAG_RT /d1/projects/MET/MET_test_data/unit_test/tc_data/diag/cira_diag_rt/2022/GFSI/sal092022_avni_doper_20220926*_diag.dat -diag CIRA_DIAG_RT /d1/projects/MET/MET_test_data/unit_test/tc_data/diag/cira_diag_rt/2022/EMXI/sal092022_emxi_doper_20220926*_diag.dat -config /d1/personal/linden/git/feature_2476_tc_pairs_diag/MET/internal/test_unit/config/TCPairsConfig_DIAGNOSTICS_CONSENSUS -out al092022_20220926_DIAGNOSTICS_CONSENSUS

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 and look specifically at the output for the tc_pairs_DIAGNOSTICS_CONSENSUS unit test

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

Yes a new unit test was added to unit_tc_pairs.xml (tc_pairs_DIAGNOSTICS_CONSENSUS)

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

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

Yes, this change added consensus (average) diagnostic values to the output file

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

…_DIAG_RT sources. If left empty, the ATCF ID of the corresponding tracks is read from the CIRA diagnostics input file.

…mputation of a track consensus.

…ues for consensus track. SL. ci-skip-all

…. SL

…es after initializing it with the first track point.

… a value based on the diag name. SL ci-skip-all

…some actual values. SL ci-skip-all

…g_name(). Added some debugging. SL

…lues. SL. ci-skip-all

…into feature_2476_tc_pairs_diag

…. SL

internal/test_unit/xml/unit_tc_pairs.xml

src/libcode/vx_tc_util/track_info.cc

src/libcode/vx_tc_util/track_info.h

JohnHalleyGotway · 2023-10-02T17:53:28Z

src/libcode/vx_tc_util/track_info.cc

+         mlog << Debug(3) << "  cons_diag_val = " << cons_diag_val << "\n";
+
+         // Add the cons_diag_val to the pavg DiagVal NumArray (using add_diag_value)
+         if(!is_bad_data(cons_diag_val)) {


Don't we actually NEED to store the computed diagnostic value regardless of whether or not it contains bad data? For example, let's say all the input tracks report a diag value with bad data. If we fail to write bad data to the output, we'll end up with a mismatch of diagnostic names and values.

Hi John, I wasn't sure if we wanted to save the missing data. But based on what you are saying it makes sense.

JohnHalleyGotway

Seth, thanks for the PR. I proposed several cleanup changes, removing whitespace, commented-out code, and overly detail log messages that are useful for development but overkill for users.

The only substantive change is the handling of missing data. I think we should store a computed diagnostic value for each diagnostic name, regardless of whether it contains good or bad data. If we don't store bad data, we'll end up with a mismatch of diagnostic names and values.

sethlinden · 2023-10-02T19:20:17Z

@JohnHalleyGotway thanks for the thorough review and proposing the code changes. All your changes look reasonable to me including removing some of the irrelevant debug / log messages. What's the best way to proceed? Do I just look at the suggested changes, here (via GitHub online) and then accept the changes (to get them into the feature branch)?

JohnHalleyGotway · 2023-10-02T20:25:27Z

Seth, yep, you could use the "commit suggestion" button next to each change. Although that may trigger many runs of the GHA testing workflow. And I didn't make a code suggestion for stashing the diag value even if its bad data. So you'll need to make that change in your local branch and push it.

Because of that, I'd recommend just making all these changes locally, doing a single commit, and then pushing up that commit. That'll prevents lots of unneeded GHA runs.

Co-authored-by: John Halley Gotway <johnhg@ucar.edu>

… loop that calculates and stores the consensus diag value, modified to store missing values as well as non-missing values. SL

sethlinden · 2023-10-02T21:19:40Z

@JohnHalleyGotway I basically did a combination of both. I used the "commit suggestion" for most of the changes above (blank spaces, debug changes)....on a side note it didn't initially like one of your suggested debug/log changes. I fixed it in my branch (from seneca).

Then I also update track_info.cc to save missing consensus debug values as well. I pushed the changes to GitHub.

I then merged latest from develop. And did a new "make clean install test".

Then I re-ran my own tc_pairs test. Everything looks good. I think this is close to being set now. Can you confirm?

sethlinden · 2023-10-02T21:21:02Z

@JohnHalleyGotway the workflow testing is still failing so I need to look into that next. Once we have confirmed the code changes. I assume the changes you made to the config file and documentation made it into this branch as well (since I didn't see a commit suggestion for those changes).

JohnHalleyGotway · 2023-10-04T16:06:50Z

@sethlinden sorry, I missed the notification for this the other day. I realize that I never re-created the input test data tarfile to contain the contrived test data needed for this new tc_pairs unit test. I'd been hoping we'd replace the test, but haven't gotten there yet. So I'm recreating that tar file on mohawk now and will rerun the tests for this PR when that's done.

JohnHalleyGotway

I approve of these changes. Thanks for bearing with me @sethlinden. After updating the GHA input dataset, I note that the unit tests all ran without error.

The new TC-Pairs output file is flagged as a difference and develop-ref will need be updated to get past that change.

I'll go ahead and push the button to merge these changes.

Co-authored-by: jprestop <jpresto@ucar.edu> Co-authored-by: Seth Linden <linden@seneca.rap.ucar.edu> Co-authored-by: John Halley Gotway <johnhg@ucar.edu> Co-authored-by: Daniel Adriaansen <dadriaan@ucar.edu> Co-authored-by: John and Cindy <halleygotway@Halleys-Mac-mini.local> Co-authored-by: rgbullock <bullock@ucar.edu> Co-authored-by: Randy Bullock <bullock@seneca.rap.ucar.edu> Co-authored-by: Dave Albo <dave@seneca.rap.ucar.edu> Co-authored-by: Howard Soh <hsoh@seneca.rap.ucar.edu> Co-authored-by: George McCabe <23407799+georgemccabe@users.noreply.github.com> Co-authored-by: hsoh-u <hsoh@ucar.edu> Co-authored-by: MET Tools Test Account <met_test@seneca.rap.ucar.edu> Co-authored-by: Seth Linden <linden@ucar.edu> Co-authored-by: lisagoodrich <33230218+lisagoodrich@users.noreply.github.com> Co-authored-by: davidalbo <dave@ucar.edu> Co-authored-by: Lisa Goodrich <lisag@ucar.edu> Co-authored-by: metplus-bot <97135045+metplus-bot@users.noreply.github.com> Co-authored-by: j-opatz <59586397+j-opatz@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Jonathan Vigh <jvigh@ucar.edu> Co-authored-by: Tracy Hertneky <39317287+hertneky@users.noreply.github.com> Co-authored-by: David Albo <dave@ucar.edu> Co-authored-by: Dan Adriaansen <dadriaan@ucar.edu> fix 2518 dtypes appf docs (#2519) fix 2531 compilation errors (#2533) fix #2531 compilation_errors_configure (#2535) fix #2514 develop clang (#2563) fix #2575 develop python_convert (#2576) 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) fix #2449 develop pdf (#2464) fix #2402 develop sonarqube (#2468) fix #2426 develop buoy (#2475) fix 2596 main v11.1 rpath compilation (#2614) fix #2514 main_v11.1 clang (#2628) fix #2644 develop percentile (#2647)

JohnHalleyGotway and others added 14 commits September 7, 2023 12:58

Per #2476, clarify the match_to_track TC-Pairs config option for CIRA…

b7f9573

…_DIAG_RT sources. If left empty, the ATCF ID of the corresponding tracks is read from the CIRA diagnostics input file.

Per #2476, add a fake data example of including diagnostics in the co…

3335d58

…mputation of a track consensus.

Per issue #2476, added code to compute consensus (avg) diagnostic val…

7b623d2

…ues for consensus track. SL. ci-skip-all

Per issue #2476, moved derive_consensus() to be after process_diags()…

70219a0

…. SL

Per issue #2476, cleaned up some unused test code. SL

c99a3bd

Per #2476, when computing a consensus, clear the psum diagnostic valu…

5840c93

…es after initializing it with the first track point.

Per issue #2476, added new function get_diag_val() that will retrieve…

20d2e25

… a value based on the diag name. SL ci-skip-all

Per issue #2476, in get_diag_val, added check that DiagVal array has …

5e8587b

…some actual values. SL ci-skip-all

Per issue #2476, updated the diag code in consensus() to use tavg.dia…

4613e7c

…g_name(). Added some debugging. SL

Per issue #2476, latest version for producing consensus diagnostic va…

c122eb7

…lues. SL. ci-skip-all

Merge branch 'develop' into feature_2476_tc_pairs_diag

de73d9d

Per #2476, fix track_point.h to define the scope for std::string.

cf8644c

Merge branch 'feature_2476_tc_pairs_diag' of github.com:dtcenter/MET …

09f015f

…into feature_2476_tc_pairs_diag

Per issue #2476, updated void add_uniq_diag_name() to use std::string…

10dfdae

…. SL

sethlinden added this to the MET 12.0.0 milestone Sep 27, 2023

sethlinden requested a review from JohnHalleyGotway September 27, 2023 21:13

JohnHalleyGotway linked an issue Sep 28, 2023 that may be closed by this pull request

Enhance TC-Pairs to include storm diagnostics in consensus track output #2476

Closed

20 tasks