Skip to content

Conversation

@DPeterK
Copy link
Member

@DPeterK DPeterK commented Dec 8, 2014

Improve grib saving of shape of the earth key

Improves the setting and testing of the GRIB2 Grid Definition Section key shapeOfTheEarth.

Translation of three more key values has been added. These are (see GRIB Spec Code Table 3.2):

  • 0: "Earth assumed spherical with radius = 6 367 470.0 m".
  • 6: "Earth assumed spherical with radius of 6 371 229.0 m".
  • 9: "Earth represented by the Ordnance Survey Great Britain 1936 Datum, using the Airy 1830 Spheroid,
    the Greenwich meridian as 0 longitude, and the Newlyn datum as mean sea level, 0 height".

Some test results have been changed in line with these changes, where these changes have corrected the value that shapeOfTheEarth was previously and incorrectly being set to.

The shape of the earth tests in the tests for GDT 0, 1, 5 and 12 have also been refactored into their own test module to prevent duplicating the tests.

@marqh
Copy link
Member

marqh commented Jan 14, 2015

This all looks good to me

I have cross checked the GRIB2 specification and all the definitions match up

I like the refactoring of the unit tests so that
test_shape_of_the_earth
is its own test module; this helps a lot with comprehension.

The test coverage seems good.

I recommend this as being ready to merge

Copy link
Member

Choose a reason for hiding this comment

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

It seems odd to be setting the radius even for shape 0 and 6 which have an implicit shape...?

Copy link
Member Author

Choose a reason for hiding this comment

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

I feel that just because it's implicit doesn't mean it shouldn't be explicitly set -- the key exists in the templates regardless of the value of the shape of the earth key. This is a downside of the overspecified nature of GRIB templates... However, whether also setting the scale factor to 0 is appropriate is another question...

Copy link
Member Author

Choose a reason for hiding this comment

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

Given that there are merits to how I've implemented this above and @rhattersley's suggestion I had a look at the _load_convert code for interpreting the shapeOfTheEarth key. It just uses the value of this key to set the coord system of the the grid specified in the message's Section 3, so I'm happy to concede this point.

I'd still like some thoughts on setting the scale factor to 0 though!

@marqh marqh mentioned this pull request Feb 2, 2015
@DPeterK
Copy link
Member Author

DPeterK commented Feb 11, 2015

Review actions. I've added a couple of tests for the RotatedGeogCS coord system and stopped explicitly setting the radius keys for shapeOfTheEarth = 0, 6.

@pelson
Copy link
Member

pelson commented Mar 17, 2015

👍 - I see this as an improvement of the current handling of shapeOfEarth. @dkillick, do you want to update this PR, get the tests passing, and I'll do a thorough review + merge?

@DPeterK
Copy link
Member Author

DPeterK commented Mar 17, 2015

@pelson I'll have a play!

@DPeterK DPeterK closed this Mar 17, 2015
@DPeterK
Copy link
Member Author

DPeterK commented Mar 17, 2015

Having now had a play the test failures are non-trivial to fix, so I'm going to close this for now on account of time constraints.

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.

4 participants