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 support for Lambert Azimuthal Equal Area Projection #187

Merged
merged 22 commits into from
Jun 15, 2020

Conversation

tpowellmeto
Copy link
Contributor

Add support for loading GRIB2 template 3.140
https://apps.ecmwf.int/codes/grib/format/grib2/templates/3/140

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 86.793% when pulling a9a3de4 on tpowellmeto:LAEA into 1f859fc on SciTools:master.

@tpowellmeto
Copy link
Contributor Author

bump

@tpowellmeto
Copy link
Contributor Author

tpowellmeto commented May 15, 2020

@lbdreyer @bjlittle @alastair-gemmell Apologies for tagging you all but I wasn't sure who to contact to review and I'm not able to assign reviewers. This one has been hanging around since Jan, is there anything extra that needs doing before this gets merged? This is required to load MO UKV GRIB files into Iris.

@@ -143,6 +143,9 @@ def data(self):
raise TranslationError(msg)
if template in (20, 30, 90):
shape = (grid_section['Ny'], grid_section['Nx'])
elif template == 140:
shape = (grid_section['numberOfPointsAlongYAxis'],
grid_section['numberOfPointsAlongXAxis'])
Copy link
Member

Choose a reason for hiding this comment

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

Have you checked that we can't just use "Nx" and "Ny", and so integrate this into the section above ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

noted, unfortunately when I tried this it didn't work hence adding these more complete names.

nx = section['Nx']
ny = section['Ny']
if section['gridDefinitionTemplateNumber'] == 140:
dx = section['xDirectionGridLengthInMillimetres']
Copy link
Member

Choose a reason for hiding this comment

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

Did you check that the shorter names don't in fact work?
-- if not, we wouldn't need this test, or this section because the generic ("else") code can do it.

I ask because I can see that the template definition 3.140 in the official docs (https://community.wmo.int/activity-areas/wmo-codes/manual-codes/volume-i2) does at least reference the shorter names,
e.g. "31–34 Nx – number of points along the x-axis".

In the end though, we can only use what implemented in eccodes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as above

@pp-mo
Copy link
Member

pp-mo commented May 27, 2020

@tpowellmeto Sorry for raising points after making you wait for ages
-- we are ready to get this done now !

Your test is very tidy, thanks.

@pp-mo
Copy link
Member

pp-mo commented May 27, 2020

@tpowellmeto I'm just wondering if you also have some "real data" which you could release in public (in iris-test-data), so we can have a practical fallback when doing code development ?
It would need to be something where a single message is reasonably small, say a few Mb at most.

Whereas a focussed unit-test is very useful for picking up odd mistakes and changes, "real" data is needed to support integration testing, which is invaluable if code needs refactoring.

I say this because we have had many problems in the past with GRIB compatibility
-- I think because the specs are so difficult to interpret precisely.
We write new templates, and then people find files for which they don't work !

@tpowellmeto
Copy link
Contributor Author

@pp-mo WRT the request for "real-data", unfortunately I don't have the authority off the bat to release operational data into the wild. I will take this as an action and follow up with Service Hub owners to see what is possible. So as not to block this change if I can get some I'll raise as a future PR. :)

@pp-mo pp-mo merged commit 2e99443 into SciTools:master Jun 15, 2020
@pp-mo
Copy link
Member

pp-mo commented Jun 15, 2020

Thanks @tpowellmeto !

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