Skip to content

Conversation

JohnHalleyGotway
Copy link
Collaborator

@JohnHalleyGotway JohnHalleyGotway commented Apr 26, 2023

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:

    This changes the dimension order for the NetCDF files written by the TC-RMW tool to list the time dimension first (i.e. track point), the vertical level dimension second if applicable (i.e. pressure), range third, and azimuth last. For this work, I made changes to library code, tc_rmw application code, rmw_analysis application code, and the docs.

NOTE: The rmw_analysis code is NOT smart enough to handle both dimension orders. If you pass it data in an order it doesn't expect, it'll error out like this:

DEBUG 2: Processing PRMSL
terminate called after throwing an instance of 'netCDF::exceptions::NcEdge'
  what():  NetCDF: Start+count exceeds dimension bound
file: ncVar.cpp  line:1622
FATAL: Received Signal Abort. Exiting 6

Personally, I don't think it's worth investing time and effort to make rmw_analysis backward compatible.

But @KathrynNewman if you tell me otherwise, I could work on it.

Pull Request Testing

  • Describe testing already performed for these changes:

    Confirmed that unit_tc_rmw.xml and unit_rmw_analysis.xml both work as expected.

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

  • Inspect the code changes.

  • Confirm all the unit tests run without error via GHA.

  • The GHA tests should flag the diffs and confirm that the difference is just the order of the dimensions.

  • Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [Yes]
    While I did update the order of dimensions listed in the documentation, the documentation for the TC-RMW and RMW-Analysis tools is rather sparse.

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

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

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

    Changes the NetCDF output from the TC-RMW and RMW-Analysis tools.

  • Please complete this pull request review by [Fri 4/28/23].

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.

…_rmw tool. The dimensions should have time first, vertical level second (if applicable), and then gridded dimensions (i.e. lat,lon) last.
@JohnHalleyGotway JohnHalleyGotway added this to the MET 11.1.0 milestone Apr 26, 2023
@JohnHalleyGotway JohnHalleyGotway marked this pull request as ready for review April 27, 2023 17:19
@JohnHalleyGotway JohnHalleyGotway requested review from jprestop and removed request for KathrynNewman May 8, 2023 16:32
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 have inspected the code changes and have confirmed that all the unit tests ran without error via GHA, except the one failed test that was expected which flagged the diffs. I have confirmed that the errors were simply that "NetCDF headers differ" and that there were no other errors. I approve this pull request. Thanks for all of your work on this @JohnHalleyGotway.

@JohnHalleyGotway JohnHalleyGotway merged commit 0502262 into develop May 10, 2023
@JohnHalleyGotway JohnHalleyGotway deleted the feature_2523_dim_order branch May 10, 2023 19:51
JohnHalleyGotway added a commit to dtcenter/METplus that referenced this pull request May 11, 2023
Co-authored-by: Julie Prestopnik <jpresto@ucar.edu>
Co-authored-by: johnhg <johnhg@ucar.edu>
Co-authored-by: cristianastan2 <cstan@gmu.edu>
Co-authored-by: John Halley Gotway <johnhg@ucar.edu>
Co-authored-by: bikegeek <minnawin@ucar.edu>
Co-authored-by: Lisa Goodrich <lisag@ucar.edu>
Co-authored-by: Julie Prestopnik <jpresto@seneca.rap.ucar.edu>
Co-authored-by: George McCabe <23407799+georgemccabe@users.noreply.github.com>
Co-authored-by: Hank Fisher <fisherh@ucar.edu>
Co-authored-by: Dan Adriaansen <dadriaan@ucar.edu>
Co-authored-by: jprestop <jpresto@ucar.edu>
Co-authored-by: Tracy Hertneky <hertneky@seneca.rap.ucar.edu>
Co-authored-by: Giovanni Rosa <giovanni.rosa@unimol.it>
Co-authored-by: j-opatz <59586397+j-opatz@users.noreply.github.com>
Co-authored-by: lisagoodrich <33230218+lisagoodrich@users.noreply.github.com>
Co-authored-by: bikegeek <3753118+bikegeek@users.noreply.github.com>
Co-authored-by: j-opatz <jopatz@ucar.edu>
Co-authored-by: Will Mayfield <59745143+willmayfield@users.noreply.github.com>
Co-authored-by: metplus-bot <97135045+metplus-bot@users.noreply.github.com>
Co-authored-by: Tracy Hertneky <39317287+hertneky@users.noreply.github.com>
Co-authored-by: Giovanni Rosa <g.rosa1@studenti.unimol.it>
fixing errors #650
fixing errors take 2 #650
fixing errors take 3 #650
fixing errors take 4 #650
fixing errors take 5 #650
fixing question order #650
fix #1706 fix PhaseDiagram use case to avoid writing into INPUT_BASE (#1708)
fix #1713 develop METPLOTPY_BASE (#1715)
fix #1691 remove whitespace from output file paths (#1721)
fix Contributor's Guide GitHub Workflow page (#1774)
fix release (#1790)
fix GitHub Actions warnings (#1864)
fix #1884 develop PCPCombine {custom} in subtract method (#1887)
fix #1939 develop - failure reading obs when zipped file also exists (#1941)
Closes #1986
fix develop Fix broken documentation links (#2004)
fix #2026 develop StatAnalysis looping (#2028)
fix priority of obs_window config variables so that wrapper-specific version is preferred over generic OBS_WINDOW_BEGIN/END (#2062)
fix #2070 var list numeric order (#2072)
fix #2087 develop docs_pdf (#2091)
fix #2096/#2098 develop - fix skip if output exists and do not error if no commands were run (#2099)
Fix for Dockerfile smell DL4000 (#2112)
fix #2082 develop regrid.convert/censor_thresh/censor_val (#2140)
fix #2082 main_v5.0 regrid.convert/censor_thresh/censor_val (#2101)
fix #2137 develop PointStat -obs_valid_beg/end (#2141)
fix failured introduced by urllib3 (see urllib3/urllib3#2168)
fix #2161 develop PCPCombine additional field arguments in -subtract mode (#2162)
JohnHalleyGotway added a commit that referenced this pull request May 11, 2023
Co-authored-by: jprestop <jpresto@ucar.edu>
Co-authored-by: John Halley Gotway <johnhg@ucar.edu>
Co-authored-by: Seth Linden <linden@seneca.rap.ucar.edu>
Co-authored-by: Daniel Adriaansen <dadriaan@ucar.edu>
Co-authored-by: hsoh-u <hsoh@ucar.edu>
Co-authored-by: George McCabe <23407799+georgemccabe@users.noreply.github.com>
Co-authored-by: johnhg <johnhg@ucar.edu>
Co-authored-by: Howard Soh <hsoh@seneca.rap.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>
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)
fix #2449 develop pdf (#2464)
fix #2402 develop sonarqube (#2468)
fix #2426 develop buoy (#2475)
fix 2518 dtypes appf docs (#2519)
fix 2531 compilation errors (#2533)
fix #2531 compilation_errors_configure (#2535)
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-RMW to reorder the dimensions of the NetCDF output to store the gridded dimensions last
2 participants