-
Notifications
You must be signed in to change notification settings - Fork 24
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 #2565 nccf_laea #2616
Feature #2565 nccf_laea #2616
Conversation
… from CF-compliant NetCDF files. Note that the GRIB2 parameters remain the primary method for defining LAEA grids. Randy uses the NetCDF parameters to instantiate the GRIB2 data structure.
… adopting this as the standard way of initializing an LAEA grid.
…c to dump the grid data structs that are parsed, just like we do for other file formats (GRIB2, MET NetCDF, ...)
…nd error out if they're non-zero.
…ide semi_major_axis and semi_minor_axis by 2.
…place the sample MetOffice data over the UK.
…rt-coming in the grib2c library in handling negative longitude values. Define and call a utility function to reprocess the bytes and check for negative values.
…GHA build artifacts. After using docker to build the image, run a docker copy command to copy out the compliation logs.
|
||
laea.nx = gfld->igdtmpl[7]; | ||
laea.ny = gfld->igdtmpl[8]; | ||
laea.lat_first = (double)gfld->igdtmpl[9] / 1000000.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if it's worth it, particularly with the time crunch, but I was wondering if we should consider creating a variable for the 1000000.0 value, which is reused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point Julie. Thanks for mentioning that. I'd like to not make a change at this time because it would apply to about 40 lines of code in this file. I'd rather limit the changes to what's explicitly necessary for this issue during the official release testing time period to limit the lines of code I touch. But this would be good to consider as part of the changes to the vx_grid library's use of the proj library in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed the code changes, reviewed the small change to the documentation, noted the differences in the unit tests as described in the PR description, and downloaded the artifacts from this GHA run and confirmed that the logs now include the log output from all compilation steps. Thanks for getting that working, @JohnHalleyGotway. Having those logs will be very helpful. I approve this PR.
Expected Differences
This Pull Request consists of 3 main changes:
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)? [No]
If yes, please describe:
Pull Request Testing
Describe testing already performed for these changes:
Manually tested with CF-compliant NetCDF and GRIB2 inputs and confirmed that they work as expected.
Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:
Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [Yes]
Do these changes include sufficient testing updates? [Yes]
Added a new LAEA test for cf-compliant NetCDF data.
Will this PR result in changes to the test suite? [Yes]
If yes, describe the new output and/or changes to the existing output:
All the unit tests run without error but 2 differences are flagged.
Compare this to the plot of this data produced by panoply:
Old on the left and new on the right:
The difference is an ever so subtle change in the orientation of the map outlines. They used to be tilted more left and are now more upright. This is caused by a fix to the central longitude value that's parse for this grid:
Old wrong value:
New correct value:
That value of 2.5 is confirmed correct in the output from wgrib2 (ignore the difference in sign... that's degrees east vs west):
Note that I submitted this as an issue in the grib2c GitHub repo:
NOAA-EMC/NCEPLIBS-g2c#427
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