name to grib #835

bblay · 2013-11-12T16:26:46Z

Reviewer: cpelley

Enhance NAME to GRIB translation, such that comparable cubes can be loaded back in.
The internal user story is as follows:

>>> for i, cube in enumerate(iris.load(filename)): 
…        iris.save(cube, 'cube{}.grib2'.format(i))
should result in grib files that can be loaded back into Iris resulting in comparable cubes to the
source i.e. equal data and coords where filename is NAMEII_field.txt and also NAMEIII_field.txt
from the current test data. The name and units should be preserved if supported by GRIB2, and
set to unknown/missing otherwise.

TODO:

default grib save param #833 must to be merged for this branch to work. Rebasing onto that branch gives fully passing tests.
~~[ ] pr metarelate~~

Raised https://github.com/SciTools/iris-code-generators/pull/23 to mirror the rule changes...

cpelley · 2013-11-13T15:01:42Z

lib/iris/fileformats/grib/grib_save_rules.py

@@ -393,7 +393,7 @@ def type_of_statistical_processing(cube, grib, coord):
    if len(coord_names) == 1 and coord_names[0] == coord.name():
        if cell_method.method == 'mean':
            stat_code = 0
-        elif cell_method.method == 'accumulation':
+        elif cell_method.method in ['accumulation', 'sum']:


Since your in this space anyway, 'accumulation' doesn't look like it's a valid cf cell method. Is this captured in any of the tests? Might be worth removing it.

Actually... while you're here, why not do a dictionary lookup?

cpelley · 2013-11-13T15:32:26Z

Why is this PR limited to NAMEII/III fields? What about NAMEIII trajectories for example, is it not applicable here too?

cpelley · 2013-11-13T15:37:32Z

lib/iris/tests/integration/format_interop/test_name_grib.py

+                # Prepare for comparison
+                grib_cube.coord('forecast_period').convert_units('minutes')
+                name_cube.rename('unknown')
+                name_cube.units = 'unknown'


I'll take a close look at these comparisons once #833 is in.

doing so now

cpelley · 2013-11-13T16:00:27Z

I think this PR definitely warrants adding information to the whatsnew. Mentioning that cell methods are now created and also support for grib export from NAME.

cpelley · 2013-11-13T16:05:39Z

lib/iris/fileformats/grib/load_rules.py

@@ -318,6 +318,18 @@ def convert(grib):

    if \
            (grib.edition == 2) and \
+            (grib.typeOfFirstFixedSurface == 103) and \


For my own reference, this corresponds to 'Specified height level above ground'

cpelley · 2013-11-13T16:26:26Z

Over to you @bblay

cpelley · 2013-11-14T09:18:04Z

@rhattersley - Ideally, all code changes should be accompanied by one or more unit tests, and by zero or more integration tests. - #818

I'm quite happy with the integration tests here, I would have unit tested the grib load rules added by mocking out the grib API (which I believe is already in the iris codebase ) and also unittesting the changes made within lib/iris/fileformats/name_loaders.py

What do you think @rhattersley?

bblay · 2013-11-14T14:16:21Z

Why is this PR limited to NAMEII/III fields?

That's just the user story I'm working on , as pasted into this PR's description.

bblay · 2013-11-14T14:48:54Z

I would have unit tested the grib load rules added by mocking out the grib API ... and also unittesting the changes made within lib/iris/fileformats/name_loaders.py

Fair comment. I was probably wrong to omit those tests, but everything you mention is actually being tested already, which gives an interesting effort vs benefit question. I thought I'd see where that lead.

For starters, it's not too safe, as it relies on indirect tests which could change.
E.g the grib load changes are tested when the saved name cubes are loaded back in (pending other review action!),
but that might change one day, and then there'd be no testing of it...I'll add the tests.

bblay · 2013-11-14T15:16:20Z

Review actions + rebased onto latest master.

I'll add the tests.

I tested GRIB2 height level loading, but only one of the two new rules as it looked horribly mockist otherwise :D, and they're pretty much the same. For the name loading, that's properly tested already, as we can see in the results.

cpelley · 2013-11-14T16:45:49Z

lib/iris/fileformats/grib/grib_save_rules.py

@@ -393,7 +393,7 @@ def type_of_statistical_processing(cube, grib, coord):
    if len(coord_names) == 1 and coord_names[0] == coord.name():
        if cell_method.method == 'mean':
            stat_code = 0
-        elif cell_method.method == 'accumulation':
+        elif cell_method.method == 'sum':


Any reason, why you object to a dictionary lookup? :)

I've done that, I just haven't pushed any of the review actions since the original request.

cpelley · 2013-11-14T16:54:51Z

You have not taken action on the issues surrounding your coordinates:

>>> coord='time'; print name_cube.coord(coord); print grib_cube.coord(coord)
DimCoord([2010-01-26 12:00:00], bounds=[[2010-01-26 09:00:00, 2010-01-26 12:00:00]], standard_name='time', calendar='gregorian')
DimCoord([2010-01-26 10:30:00], bounds=[[2010-01-26 09:00:00, 2010-01-26 12:00:00]], standard_name='time', calendar='gregorian')
>>> coord='height'; print name_cube.coord(coord); print grib_cube.coord(coord)
DimCoord(array([ 50.]), bounds=array([[   0.,  100.]]), standard_name='height', units=Unit('m'))
DimCoord(array([ 50.]), bounds=array([[   0.,  100.]]), standard_name=None, units=Unit('m'), long_name='height')

You will need to correct this and address your tests which don't currently compare the coordinates due to the logic you have used.

Fair comment. I was probably wrong to omit those tests, but everything you mention is actually being tested already, which gives an interesting effort vs benefit question. I thought I'd see where that lead.

I'm now more inclined to conduct unittesting :)
Unittesting and at at time mocking objects allows us to assert calling of functions/methods and ensures that the modification you make result in behaviour that you explicitly intended (this was not captured in your integration tests here). I only discovered issued when debugging a 'passing' test.

cpelley · 2013-11-14T16:58:16Z

lib/iris/tests/integration/format_interop/test_name_grib.py

+                self.assertTrue(np.allclose(name_cube.data, grib_cube.data))
+                self.assertTrue(name_cube.is_compatible(grib_cube))
+                for coord in name_cube.coords():
+                    if grib_cube.coords(coord):


Your tests will pass whatever happens, as this won't be True

cpelley · 2013-11-14T17:02:47Z

Over to you @bblay

bblay · 2013-11-15T15:07:03Z

You have not taken action on the issues surrounding your coordinates

I haven't push any of the new review actions yet! 😀

bblay · 2013-11-19T14:06:35Z

There has been quite a big change in the way time is handled here, so I've made a new, single commit as a new starting point.

More specifically, I was not interpreting the NAME dates properly, confusing "run time" with "model start time". We can't actually get the model start time, so we can't calculate a CF-style forecast_period on loading NAME fields. This has the knock on effect of not being able to save to grib2 without changes to the grib saving ruies, which are in this new commit.

bblay · 2013-11-19T14:22:19Z

lib/iris/tests/integration/format_interop/test_name_grib.py

+                        ('integration', 'name_grib', 'NAMEII',
+                         '{}_{}.cml'.format(i, name_cube.name()))))
+
+    def test_name3_field(self):


This is pretty much identical to the name2 test, happy to squidge them together if preferred.

I would have a private method which puts all this common stuff together, but still have two separate unittest functions, one for the NAMEII and the other for the NAMEIII which calls it.

In future, I would not have added these 'acceptance tests', these are internally driven and should not replace a proper coverage using unittests.

You have not pulled out the common code into a private method of the class. In addition, I would also mention that these integration tests will be moved into internal acceptance tests in future.

bblay · 2013-11-19T14:36:28Z

Back to you, @cpelley

bblay · 2013-11-20T08:38:21Z

just getting set up at home to fix that test...

cpelley · 2013-11-21T14:13:46Z

lib/iris/fileformats/grib/grib_save_rules.py

+    if fp_coord is not None:
+        if fp_coord.has_bounds():
+            raise iris.exceptions.TranslationError(
+                "Bounds not expected for 'forecast_period'")


Might be worth changing this message, the problem is with interpreting a bounded forecast period when exporting to GRIB specifically, is it not?

scratch that, the exception will come up in grib_save_rules

cpelley · 2013-11-21T16:02:09Z

lib/iris/tests/test_grib_load.py

+        self.assertEqual((coord.name(), coord.units, coord.points[0]),
+                         ('height', 'm', 2.0))
+
+    def test_grib2_height(self):


move these to unittests dir

cpelley · 2013-11-22T10:05:59Z

In future can you put the cml changes in a separate commit :)

cpelley · 2013-11-22T14:50:19Z

lib/iris/fileformats/grib/grib_save_rules.py

-            stat_code = 6
+        stat_codes = {'mean': 0, 'sum': 1, 'maximum': 2, 'minimum': 3,
+                      'standard_deviation': 6}
+        stat_code = stat_codes[cell_method.method]


The type_of_statistical_processing is still not being unittested

cpelley · 2013-11-22T14:59:03Z

Technically speaking, this is also the time to build the mock framework for both NameII and NameIII fields as unittests since you should check that _generate_cubes is being called with your cell_methods.

cpelley · 2013-11-22T15:00:49Z

lib/iris/tests/integration/format_interop/test_name_grib.py

+
+class TestNameToGRIB(tests.IrisTest):
+    def _assert_cube_equal(self, path_name, path_result):
+        pass


_assert_cube_equal not being used

bblay · 2013-11-22T15:21:05Z

Technically speaking, this is also the time to build the mock framework for both NameII and NameIII fields as unittests

Just checking, that's not a request to do that now, right?

cpelley · 2013-11-22T15:22:55Z

Well you have changed them, I would have

cpelley · 2013-11-22T16:52:26Z

Just to mention.. this PR has now substantially increased in size, another side-effect of stringent unit-testing.

cpelley · 2013-11-22T17:17:25Z

lib/iris/tests/integration/format_interop/test_name_grib.py

+class TestNameToGRIB(tests.IrisTest):
+
+    def check_common(self, name_cube, grib_cube):
+        self.assertTrue(np.allclose(name_cube.data, name_cube.data))


I would have used, the iris self.assertArrayAlmostEqua(a, b)
This results in a test failure with the latitude, longitude coords however.

On discussion with you, you have confirmed that the difference in precision can be accounted for in the way that grib packs data.

cpelley · 2013-11-22T17:21:07Z

OK, happy for merging

name to grib

ghost assigned bblay Nov 13, 2013

cpelley reviewed Nov 13, 2013
View reviewed changes

bblay closed this Nov 14, 2013

bblay reopened this Nov 14, 2013

cpelley reviewed Nov 14, 2013
View reviewed changes

bblay reviewed Nov 19, 2013
View reviewed changes

bblay closed this Nov 20, 2013

cpelley reviewed Nov 21, 2013
View reviewed changes

bblay mentioned this pull request Nov 21, 2013

Mirror Iris #835 changes metarelate/metOcean-mapping#50

Closed

cpelley reviewed Nov 21, 2013
View reviewed changes

cpelley reviewed Nov 22, 2013
View reviewed changes

bblay added 2 commits November 22, 2013 16:41

name to grib

364c007

review actions

c78686a

cpelley reviewed Nov 22, 2013
View reviewed changes

bblay added a commit that referenced this pull request Nov 22, 2013

Merge pull request #835 from bblay/name_to_grib

20ea1f0

name to grib

bblay merged commit 20ea1f0 into SciTools:master Nov 22, 2013

esc24 mentioned this pull request Mar 6, 2014

Allowed no time averaging as a column heading in NAME files #1058

Merged

name to grib #835

name to grib #835

Conversation

bblay commented Nov 12, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpelley commented Nov 13, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpelley commented Nov 13, 2013

Choose a reason for hiding this comment

cpelley commented Nov 13, 2013

cpelley commented Nov 14, 2013

bblay commented Nov 14, 2013

bblay commented Nov 14, 2013

bblay commented Nov 14, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpelley commented Nov 14, 2013

Choose a reason for hiding this comment

cpelley commented Nov 14, 2013

bblay commented Nov 15, 2013

bblay commented Nov 19, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bblay commented Nov 19, 2013

bblay commented Nov 20, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpelley commented Nov 22, 2013

Choose a reason for hiding this comment

cpelley commented Nov 22, 2013

Choose a reason for hiding this comment

bblay commented Nov 22, 2013

cpelley commented Nov 22, 2013

cpelley commented Nov 22, 2013

Choose a reason for hiding this comment

cpelley commented Nov 22, 2013