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 #2321 tc_diag_changes #2347

Merged
merged 18 commits into from
Nov 16, 2022
Merged

Conversation

JohnHalleyGotway
Copy link
Collaborator

@JohnHalleyGotway JohnHalleyGotway commented Nov 15, 2022

Expected Differences

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

    If yes, please describe:

    • Switch to using diagnostic source names CIRA_DIAG_RT and SHIPS_DIAG_RT.
    • Adds TC-Pairs diag_info_map configuration option.
    • Move diag_name config option inside the larger diag_info_map option to define diagnostics metadata.
    • Rename diag_convert_map.source to diag_convert_map.diag_source for consistency, and support partial string matching in that context.
    • Remove model=string sub-option for the -diag command line option since it's replaced by diag_info_map.match_to_track.
  • 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:

    • Add 2 new TCDIAG output columns for TRACK_SOURCE and FIELD_SOURCE.

Pull Request Testing

  • Describe testing already performed for these changes:

    • Compiled locally and ran unit_tc_pairs.xml to confirm that they all run.
    • Will use GHA for this PR to update the input test dataset and check for unexpected diffs.
  • Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:

    • @KathrynNewman please review configuration file changes, user's guide updates, and also test on seneca:
      /d1/projects/MET/MET_pull_requests/met-11.0.0/met-11.0.0-beta5/feature_2321/MET-feature_2321_tc_diag_changes
    • @jvigh please review diagnostic settings in the TC-Pairs config file diag_info_map and confirm that these are correct.
    • @georgemccabe please review the code changes and configuration file updates.
  • Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [Yes]

    • Updates the TC-Pairs documentation about diagnostics by merging changes from @jvigh's branch.
    • Updates the description of TC-Pairs command line options, configuration options, and the TCDIAG line type.
  • Do these changes include sufficient testing updates? [Yes]

    • I kept the same test in place, but I reorganized the input diagnostics data files to match our updated naming conventions.
  • 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 2 new output columns to the TCDIAG line type.
    • One TC-Pairs output file is modified (al092022_20220926_DIAGNOSTICS.tcst), but only by the addition of the TRACK_SOURCE and FIELD_SOURCE columns. The actual data values are NOT changed.
  • Please complete this pull request review by [Wed 11/16/2022].

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

@JohnHalleyGotway JohnHalleyGotway added this to the MET 11.0.0 milestone Nov 15, 2022
@JohnHalleyGotway JohnHalleyGotway linked an issue Nov 15, 2022 that may be closed by this pull request
31 tasks
Copy link
Collaborator

@georgemccabe georgemccabe left a comment

Choose a reason for hiding this comment

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

These code changes look good to me. I didn't see anything glaringly incorrect. I will use this info to update the METplus logic for PR dtcenter/METplus#1932

@JohnHalleyGotway
Copy link
Collaborator Author

I inspected the differences flagged in this GHA run. And I see that there is a problem in the TCDIAG output written by the TC-Stat tool.

I'll work on fixing that now.

Copy link
Contributor

@jvigh jvigh left a comment

Choose a reason for hiding this comment

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

I just had one minor edit for clarity in the documentation section.

Copy link
Contributor

@KathrynNewman KathrynNewman left a comment

Choose a reason for hiding this comment

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

The default settings for diag_source="SHIPS_DIAG_RT"; are:
track_source = "OFCL";
match_to_track=["OFCL"];

This results in warning messages that the track locations do not match the diagnostic location. This may result in confusion for a user running with the default settings.

After discussion with @JohnHalleyGotway - proposed changes are to only print a warning about locations NOT matching when diag_info_map.track_source = diag_info_map.match_to_track. When they do not match, print a DEBUG(4) log message.

The default track_source needs to change for SHIPS_DIAG_RT for this to work. John consulted Jonathan for advice on what to use for this string.

…message only when TrackSource = Technique. Otherwise, print it as Debug(4). Update the SHIPS_DIAG_RT track_source from OFCL to SHIPS_TRK and modify the documentation to clarify.
@JohnHalleyGotway
Copy link
Collaborator Author

Thanks for the feedback @KathrynNewman. I've made the requested changes in this commit and am re-requesting your review.

Copy link
Contributor

@KathrynNewman KathrynNewman left a comment

Choose a reason for hiding this comment

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

Mark says: "The SHIPS model includes its own "tracker" - so, rather than the SHIPS model itself, I recommend stating along a GFS forecast track defined by a SHIPS specific tracker. (or internal SHIPS tracker?) @jvigh, do you agree? The disclaimer that the SHIPS tracker doesn't appear in the NHC adeck is important (good add!).

@JohnHalleyGotway
Copy link
Collaborator Author

Done:

  • SHIPS_DIAG_RT: Real-time SHIPS diagnostics computed from a NWP model such as the Global Forecast System (GFS) model along a GFS forecast track defined by a SHIPS-specific tracker. Note that these SHIPS-derived forecast tracks do not appear in the NHC adeck data.

Copy link
Contributor

@KathrynNewman KathrynNewman left a comment

Choose a reason for hiding this comment

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

Looks good - thanks!

@JohnHalleyGotway JohnHalleyGotway merged commit 9fdd73b into develop Nov 16, 2022
@JohnHalleyGotway JohnHalleyGotway deleted the feature_2321_tc_diag_changes branch November 16, 2022 20:53
JohnHalleyGotway added a commit that referenced this pull request Nov 16, 2022
Co-authored-by: Seth Linden <linden@seneca.rap.ucar.edu>
Co-authored-by: John Halley Gotway <johnhg@ucar.edu>
Co-authored-by: MET Tools Test Account <met_test@seneca.rap.ucar.edu>
Co-authored-by: Howard Soh <hsoh@seneca.rap.ucar.edu>
Co-authored-by: Dave Albo <dave@seneca.rap.ucar.edu>
Co-authored-by: johnhg <johnhg@ucar.edu>
Co-authored-by: Lisa Goodrich <lisag@seneca.rap.ucar.edu>
Co-authored-by: jprestop <jpresto@ucar.edu>
Co-authored-by: j-opatz <59586397+j-opatz@users.noreply.github.com>
Co-authored-by: George McCabe <23407799+georgemccabe@users.noreply.github.com>
Co-authored-by: Julie Prestopnik <jpresto@seneca.rap.ucar.edu>
Co-authored-by: Jonathan Vigh <jvigh@ucar.edu>
Co-authored-by: Seth Linden <linden@ucar.edu>
Co-authored-by: hsoh-u <hsoh@ucar.edu>
Co-authored-by: bikegeek <3753118+bikegeek@users.noreply.github.com>
Co-authored-by: davidalbo <dave@ucar.edu>
Co-authored-by: lisagoodrich <33230218+lisagoodrich@users.noreply.github.com>
@jvigh
Copy link
Contributor

jvigh commented Nov 16, 2022 via email

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
4 participants