Skip to content
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

Add GRIB product definition template 40 #2020

Merged
merged 1 commit into from
May 19, 2016
Merged

Conversation

lbdreyer
Copy link
Member

This adds support for product definition template 40.

The only difference between pdt 0 and 40 is the inclusion of "constituent type".

Grib does have a mapping for the constituent type value (e.g. 0 is ozone) but for now this mapping hasn't been implemented. Instead this PR just sets the value as an attribute of the cube.

Saving to pdt 40 to come in the next commit...

@lbdreyer
Copy link
Member Author

Concerningly, the test file I have with pdt 40 doesn't have a "ForecastTime" despite the grib spec saying there should be one.

@lbdreyer
Copy link
Member Author

It looks like "forecastTime" is being encoded as "startStep":

>>> import gribapi
>>> grib_message = gribapi.grib_new_from_samples("GRIB2")
>>> gribapi.grib_set_long(grib_message, 'productDefinitionTemplateNumber', 40)
>>> gribapi.grib_set_long(grib_message, 'forecastTime', 4)
>>> with open('pdt40.grib2', 'w') as fout:
...     gribapi.grib_write(grib_message, fout)
... 
>>> gribapi.grib_release(grib_message)
>>> 
$ grib_dump -O pdt40.grib2 | grep 'forecastTime'
$ grib_dump -O pdt40.grib2 | grep 'startStep'
21-24     startStep = 4

@lbdreyer lbdreyer force-pushed the grib_pdt_40 branch 2 times, most recently from c22524e to cb760e5 Compare May 17, 2016 16:08
constituent_type = section['constituentType']

# Add the constituent type as an attribute.
metadata['attributes']['constituent_type'] = constituent_type
Copy link
Member

Choose a reason for hiding this comment

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

may I suggest we make this name more specific, perhaps
WMO_constituent_type
or
GRIB_constituent_type
?

especially as a code table value is being stored

@marqh
Copy link
Member

marqh commented May 17, 2016

It looks like "forecastTime" is being encoded as "startStep":

import gribapi
grib_message = gribapi.grib_new_from_samples("GRIB2")
gribapi.grib_set_long(grib_message, 'productDefinitionTemplateNumber', 40)
gribapi.grib_set_long(grib_message, 'forecastTime', 4)
with open('pdt40.grib2', 'w') as fout:
... gribapi.grib_write(grib_message, fout)
...
gribapi.grib_release(grib_message)

$ grib_dump -O pdt40.grib2 | grep 'forecastTime'
$ grib_dump -O pdt40.grib2 | grep 'startStep'
21-24 startStep = 4

I wonder if this is a mistake in the GRIB API
does the Manual on codes explicitly state
21-24 forecast time?

21–24 Forecast time in units defined by octet 20

looks like it to me

perhaps you could add a code comment that this is a suspected bug in the GRIB API, so that we can remove it later if we can get the bug fixed?

@marqh
Copy link
Member

marqh commented May 17, 2016

the testing looks great and the logic seems sound to me

i think agreeing on the name of the custom attribute is outstanding, I have a small preference for WMO as a common code table is being used

looks pretty close to me

@lbdreyer
Copy link
Member Author

I have:

  • renamed the attribute "WMO_constituent_type"
  • added a comment in _load_convert about the possible gribapi bug

@marqh
Copy link
Member

marqh commented May 19, 2016

Hi @lbdreyer

looks great, please squish and I will merge

mark

@ajdawson
Copy link
Member

How does this relate to the deprecation of grib support within iris and the move to an external package for grib handling? Should be be adding more features to a deprecated API, or are we not worrying, and once 1.10 is out we'll sync up iris.fileformats.grib and iris-grib?

@lbdreyer
Copy link
Member Author

I have added this to iris, rather than just iris-grib, as a user needs this before Iris 1.10 is due to come out.
The correct place to add new features is iris-grib (I will also be putting this code change into iris-grib) it is just that this is a special circumstance.

@lbdreyer
Copy link
Member Author

@marqh squashed and tests are passing 😃

@marqh
Copy link
Member

marqh commented May 19, 2016

ace, nice one

@marqh marqh merged commit 5e090b7 into SciTools:master May 19, 2016
@QuLogic QuLogic added this to the v1.10 milestone May 19, 2016
@QuLogic
Copy link
Member

QuLogic commented May 19, 2016

Not sure if I should tag this as 1.10 then.

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