Feature 1823 global #1928
Feature 1823 global #1928
Conversation
… consistent. This issue will require modifying these classes to wrap global grids.
… the mechanics, passing around the is_global boolean. I still need to actually update the logic to do the wrapping. I also need to update the rest of the codebase for these changes. Perhaps the grid template info should be a struct instead of several args?
…'s where it's applied to the logic... which I still need to do!
…setting was NOT getting set properly in Point-Stat and Ensemble-Stat. Need to add a call to set_interp_thresh instead of using the default >0 threshold.
…global grids where we wrap x.
…rid is created.
…to handle the is_global flag.
…ther than updating it to handle global grids.
… utility functions for point_stat, grid_stat, ensemble_stat, and shift_data_plane.
…n the x-direction for non-global grids.
…for skipping observations due to a bad interpolated forecast value.
…c the modulo functionality within python. This is needed to wrap negative X indices back to positive for global grids.
…e point here is whether or not the longitudes span 0 to 360 and has nothing to do with the extent of the latitudes. Using is_global incorrectly implies that the latitudes span -90 to 90.
….0.0 has a vertical stripe of missing data that is gone in this feature branch.
…ta rather than having to add new inputs. In MET version 10.0.0, this results in a vertical line at the wrap longitude point. But feature 1823 fixes and that demonstrates that smoothing across the wrap point works well.
…cing that does not evenly divide 360.
There was a problem hiding this comment.
These changes appear to add logic to properly handle wrapping longitudes so that data is interpolated and not lost on the edges.
There are many changes throughout the code base to support this new functionality. I don't fully appreciate all of the details of the changes, but I don't see anything that looks obviously wrong. The images that were generated with these changes included look much better that the previously generated images. New tests were added to demonstrate this new behavior.
Given that these changes do not break any of the automated tests, I approve of this pull request.
Many of the other changes appear to be formatting updates (comment blocks, indentation, opening curly brace location, etc.) and renaming of ifdef blocks to be consistent with the rest of the code base.
|
Thanks for the review @hsoh-u and @georgemccabe. Howard, I had already run a full regression test, as listed in the initial PR. And I included an explanation for all of the diffs. I checked, and all the diffs flagged in your log file (/dev/shm/hsoh/met_comp_20211001_1439.log) match the explanations I already provided. I'll proceed with the merge. |
Co-authored-by: Seth Linden <[email protected]> Co-authored-by: John Halley Gotway <[email protected]> Co-authored-by: Howard Soh <[email protected]> Co-authored-by: johnhg <[email protected]> Co-authored-by: Julie.Prestopnik <[email protected]> Co-authored-by: ericgilleland <[email protected]> Co-authored-by: George McCabe <[email protected]> Co-authored-by: John Halley Gotway <[email protected]> Co-authored-by: jprestop <[email protected]> Co-authored-by: Julie Prestopnik <[email protected]> Co-authored-by: hsoh-u <[email protected]> Co-authored-by: Keith Searight <[email protected]> Co-authored-by: Seth Linden <[email protected]> Co-authored-by: MET Tools Test Account <[email protected]> Co-authored-by: lisagoodrich <[email protected]> Co-authored-by: MET Tools Test Account <[email protected]> Co-authored-by: j-opatz <[email protected]>
Pull Request Testing
Describe testing already performed for these changes:
Manually tested to confirm that wrapping the longitudes works correctly both when processing gridded data as well as interpolating to point locations. See issue comment for an example.
Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:
Please review the code changes.
Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [Yes]
I made no documentation updates since I couldn't find any existing references for special handling of "global" datasets. There is a related discussion of the MODE shift_right option which we should update after making MODE fully support the wrapping of longitudes.
However the LatLon and RotLatLon do include the wrapLon = true/false in their output. So this will show up in the log output when running at high enough verbosity level. For example:
Also, I added a DEBUG(4) log message when the grid covers 360 degrees but the longitude spacing doesn't evenly divide it:
See testing updates/additions below.
If yes, describe the new output and/or changes to the existing output:
One file is added:
Running this command in earlier versions results in a vertical line of missing data and it's clear that the data is not smoothed over the wrap point, like this:
But here's what you get with the changes on this branch:
No vertical line and clearly the smoothing over the wrap longitude is working.
Several files are modified:
So why are we getting these bad forecast values with these changes? Here's a picture of the point data on this grid:
The PointStatConfig_APCP config file specifies a valid data threshold of 1.0:
For a 3x3 or 5x5 box along the edge of the domain, clearly some of the forecast values fall off the edge of the grid. Debugging reveals that the older version has vld_thresh set to 0.0 in interp_util.cc while the new version has it set to 1.0, as requested in the config file! Here's the line of code that was added to fix this issue:
MET/met/src/tools/core/point_stat/point_stat_conf_info.cc
Line 1054 in dcdbe17
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