Skip to content

Conversation

@DPeterK
Copy link
Member

@DPeterK DPeterK commented Dec 2, 2014

This adds functionality to Iris that allows cubes on a Transverse Mercator grid to be saved to a GRIB2 message. This functionality is contained in a new _save_rules function called grid_definition_template_12.

Please note however that this does not currently work due to a bug in GRIBAPI that means the only key in GDT12 that is specified as being a float32 by the GRIB spec is set as a signed int by the GRIBAPI template. On account of this, _save_rules raises a TranslationError when trying to save a cube with a Transverse Mercator grid to GRIB.

The other changes I've made to get this "working" are:

  • This would appear to be the first grid we've saved to GRIB that is not on a simple lat/lon grid. As such I've improved the section of fileformats/grib/__init__ that locates the horizontal dim coords of the cubes to find any horizontal dim coords rather than only Latitude and Longitude.
  • The logic for locating the ellipsoid that defines the shape of the earth for the saved GRIB message has been improved. This was done because the previous logic did not cater for a non-Rotated Pole coord system with an ellipsoid defined.
  • I've also removed two function calls from latlon_common as it turned out they weren't common to GDT12. Specifically, GDT12 does not define lat/lon for first and last grid points or dx/dy.
  • As this PR introduces using only gribapi.grib_set for writing GRIB message key values, I've added a trap for calling this function to the grib save rules tests __init__. The test module itself for this change is very short as calling grid_definition_template_12 will currently always result in a TranslationError.

Copy link
Member Author

Choose a reason for hiding this comment

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

Aah. Removing these wasn't strictly accurate - it's not that GDT12 doesn't set these, it's more that GDT12 calls these keys something different to what GDT0/1 do. I'll push a fix for this presently...

@rhattersley
Copy link
Member

The test module itself for this change is very short as calling grid_definition_template_12 will currently always result in a TranslationError.

There's no need for the test coverage to be limited by the bug in the ECMWF GRIB API. Just as you've patched grib_set and friends you can patch grib_get to behave as if it were working correctly.

@rhattersley
Copy link
Member

this does not currently work due to a bug in GRIBAPI

Have you raised this on the ECMWF's bug tracker?

@DPeterK
Copy link
Member Author

DPeterK commented Dec 2, 2014

no need for the test coverage to be limited

Good point, I'll add a commit soon that addresses this.

Have you raised this on the ECMWF's bug tracker?

Not yet but I will.

@rhattersley
Copy link
Member

bug in GRIBAPI

I have a work-around on its way...

@rhattersley
Copy link
Member

bug in GRIBAPI

I have a work-around on its way...

DPeterK#6

@DPeterK
Copy link
Member Author

DPeterK commented Dec 3, 2014

This commit fixes an issue with the horizontal grid not being set and adds some tests for saving GDT12.

While I'm waiting for the dust to settle around casting floats as signed ints and casting signed ints as unsigned ints, I've replaced the exception raised when setting the key scaleFactorAtReferencePoint with a warning so that the function grid_definition_template_12 will run.

I've also adjusted the function dx_dy on account of the fact that GDT12 does not have the same key names for dx and dy as GDT0&1. For a similar reason I've adjusted where the function latlon_first_last is called so that it is template-specific.

@DPeterK
Copy link
Member Author

DPeterK commented Dec 4, 2014

This latest commit has rebased the branch on to all the other changes that have been made since I raised this PR, it makes use of the helper functions from #1480 and #1481 and improves the tests to check that the fixes these helper functions provide are being applied correctly.

Copy link
Member

Choose a reason for hiding this comment

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

Since you generalised it, I think the names here are now rather misleading -- they are now really "X and Y" rather than "latitude and longitude".
You should rename the variables, and edit the error message.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point.

@pp-mo
Copy link
Member

pp-mo commented Dec 4, 2014

All good I think

  • apart from all those little picky comments I just threw at it !
    Getting very close now, thanks @dkillick .

@DPeterK
Copy link
Member Author

DPeterK commented Dec 4, 2014

Review actions - hitting @pp-mo's comments above.

@DPeterK DPeterK force-pushed the grib_save_gdt12 branch 2 times, most recently from 80d94b1 to 983f8af Compare December 4, 2014 16:30
pp-mo added a commit that referenced this pull request Dec 4, 2014
Add GRIB save GDT12 (Transverse Mercator)
@pp-mo pp-mo merged commit 97d7941 into SciTools:master Dec 4, 2014
@pp-mo
Copy link
Member

pp-mo commented Dec 4, 2014

Sorted !!
Thanks @dkillick

@DPeterK
Copy link
Member Author

DPeterK commented Dec 4, 2014

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants