Label unknown phenomena with a 'GRIB_CODING' attribute #156

pp-mo · 2019-09-06T17:03:19Z

Aims to address #153

coveralls · 2019-09-06T17:14:11Z

Coverage decreased (-0.7%) to 85.708% when pulling df54c96 on pp-mo:param_coding into 00a72eb on SciTools:master.

coveralls · 2019-09-06T17:14:12Z

Coverage increased (+0.4%) to 86.777% when pulling ff2bc89 on pp-mo:param_coding into 2cf099a on SciTools:master.

iris_grib/_load_convert.py

iris_grib/_save_rules.py

iris_grib/grib_phenom_translation.py

pp-mo · 2019-09-13T12:52:07Z

@ehogan I think you might be interested in this ?

pp-mo · 2019-09-13T12:53:06Z

Good to go now AFAIK !

@bjlittle @lbdreyer a distraction from Iris, but can you summon any interest in this ?

pp-mo · 2019-09-13T13:08:54Z

Some practical notes:

This is supposed to work just like the "STASH" attribute that Iris uses for loading + saving UM files into Iris (i.e. fieldsfiles or PP).
However.. when loading UM files, you always get a "STASH" attribute,
but when loading grib data, you will now only get a "GRIB_CODING" attribute when the phenomenon is unrecognised.

This is intended to avoid confusing existing loading code.
However, it will lead to results changing when we add new phenomenon codings. So that may not be ideal.

A safer way, more consistent with general Iris usage, would be ...

use a FUTURE type flag to enable GRIB_CODING attributes for all loads
and deprecate current usage
tidy up at a major release

But that's overkill for iris-grib,
isn't it ... ???

pp-mo · 2019-09-16T11:55:33Z

From discussion with @bjlittle ...

A safer way, ... use a FUTURE type flag
that's overkill for iris-grib, isn't it ... ???

Two points here :

(1) the above fuss is strictly not required, because we don't have a version 1.0 yet
(2) the existing proposal is still a bit flaky + not that safe for some reasons ...
- adding a cf-translation for a code means it will stop loading with a grib-coding attribute
- unlike Iris-STASH-attribute, the control attribute does not override on save, but is only honoured when a cf-translation is absent.

Given (1), we're probably better off implementing a mechanism identical to the iris-STASH-attribute,
as this is easier to explain + understand. That means ...

on load, all data will have the new attribute
on save, the presence of a stash-coding attribute will set the encoding + ignore any cf-translation

I'll just run off + implement that ...
( re- marking this "WIP" while I do that ... )

pp-mo · 2019-11-27T16:35:55Z

Update :
Just rebased
Still needs some test changes/fixes to accommodate the recent policy change...

iris_grib/tests/unit/save_rules/test_set_discipline_and_parameter.py

pp-mo · 2019-11-27T17:31:57Z

Thanks @lbdreyer
Adopted revised policy + fixed the PR.
Now good to go I think, please review ...

iris_grib/tests/unit/save_rules/test_set_discipline_and_parameter.py

lbdreyer · 2019-11-29T14:19:46Z

iris_grib/_save_rules.py

+    discipline, category, number = 255, 255, 255
+    identity_found = False
+
+    # First, see if we can find and interpret a 'GRIB_CODING' attribute.


I'm not convinced its a good idea to interpret the "GRIB_CODING" attribute first.

I think the standard_name/long_name should take precedence when determining the correct discipline/category/number as that it is the primary means we have of identifying a cube.

This reliance on GRIB_CODING means in future we have to take extra care with attributes and I don't want that requirement.

For example someone could take a copy of a cube, update the data array, and only update the name, then save it out as a cube. This would still have it out as the old discipline/category/number

From an offline discussion with @pp-mo, it turns out that this is how the STASH attribute is handled ⚠️ ⚠️ ⚠️

In which case, my preference is for matching that behaviour so I'm happy with this as it is.

lbdreyer

A few more review comments, including some python 2 code that needs dropping

And as discussed offline, round-trip testing may be helpful

lbdreyer · 2019-11-29T14:26:59Z

iris_grib/grib_phenom_translation.py

-import collections
+from collections import namedtuple
+import re
+import six


This file needs resolving merge conflicts

lbdreyer · 2019-11-29T14:27:50Z

iris_grib/tests/unit/grib_phenom_translation/test_grib_phenom_translation.py

 # importing anything else.
 import iris_grib.tests as tests

+import six


six should be removed now Py 2 has been dropped

This still needs removing?

Sorry I got confused + thought I'd done this. Will fix.

lbdreyer · 2019-11-29T14:44:50Z

iris_grib/tests/unit/save_rules/test_set_discipline_and_parameter.py

+# licensing details.
+"""Unit tests for `iris_grib.grib_save_rules.set_discipline_and_parameter`."""
+from __future__ import (absolute_import, division, print_function)
+from six.moves import (filter, input, map, range, zip)  # noqa


No longer need the Python 2 support

Ok. Nicer !

lbdreyer · 2019-11-29T14:51:40Z

iris_grib/grib_phenom_translation.py

    return _CF_GRIB2_TABLE[(standard_name, long_name)]
+
+
+class GribCode(namedtuple('GribCode',


GRIB is an acronym so you could argue that this should be GRIBCode

I tried 'GRIBcode'. Is that ok ?

iris_grib/grib_phenom_translation.py

iris_grib/_load_convert.py

lbdreyer · 2019-12-02T11:47:54Z

After more thinking and discussing, I propose the following format for the cube attribute:

attributes  = {
    'GRIB_PARAM': 'GRIB2:d001c001n001'}

Where GRIB_PARAM is the key, and the value follows the format GRIB?:d???c???n???

…essages.

Co-Authored-By: lbdreyer <[email protected]>

pp-mo · 2019-12-02T12:26:28Z

Rebased to fix.
Other requested changes coming ...

pp-mo · 2019-12-02T12:49:28Z

Thanks @lbdreyer.
Should be good now ?
Note that I thought of + added an extra test.

lbdreyer · 2019-12-02T14:15:53Z

iris_grib/grib_phenom_translation.py

    return _CF_GRIB2_TABLE[(standard_name, long_name)]
+
+
+class GRIBcode(namedtuple('GRIBcode',


I'm afraid I don't like GRIBcode I read that as GRI Bcode

Maybe it is best to go back to GribCode if you don't like GRIBCode
Wikipedia's article on camel links to an paper that points out a run of uppercase letters can cause problems which I think we're having with GRIBCode. So I don't think we have to strictly treat acronyms as all capital letters.

if you don't like GRIBCode

I read as "GRIBC ode". It seems to be the opposite problem to your "GRI Bcode" !
But I'll go with yours, it's not an awful problem.

pp-mo · 2019-12-02T18:10:24Z

Hi @lbdreyer.
Changed as requested.
Can we go with this ?

lbdreyer · 2019-12-03T11:32:57Z

Hi @lbdreyer.
Changed as requested.
Can we go with this ?

Hi @pp-mo

Yep this is almost good to go. Just need you to get rid of the six import

pp-mo · 2019-12-03T17:56:51Z

here it is, @lbdreyer !
Sorry for another mistake + delay.

lbdreyer · 2019-12-04T11:36:26Z

🎉 six is now removed 🎉

I'll merge this in ....

pp-mo changed the title ~~Support unknown phenomena with a 'GRIB_CODING' attribute; tests passing.~~ Support unknown phenomena with 'GRIB_CODING' attribute Sep 6, 2019

pp-mo changed the title ~~Support unknown phenomena with 'GRIB_CODING' attribute~~ Label unknown phenomena with 'GRIB_CODING' attribute Sep 6, 2019

pp-mo changed the title ~~Label unknown phenomena with 'GRIB_CODING' attribute~~ Label unknown phenomena with a 'GRIB_CODING' attribute Sep 6, 2019

pp-mo commented Sep 13, 2019

View reviewed changes

iris_grib/_load_convert.py Outdated Show resolved Hide resolved

pp-mo commented Sep 13, 2019

View reviewed changes

iris_grib/_save_rules.py Outdated Show resolved Hide resolved

pp-mo commented Sep 13, 2019

View reviewed changes

iris_grib/grib_phenom_translation.py Outdated Show resolved Hide resolved

pp-mo commented Sep 13, 2019

View reviewed changes

iris_grib/grib_phenom_translation.py Outdated Show resolved Hide resolved

pp-mo force-pushed the param_coding branch from 6c20e6e to 5a9e87c Compare September 13, 2019 12:22

pp-mo mentioned this pull request Oct 15, 2019

Add NameConstraint with relaxed name loading SciTools/iris#3463

Merged

pp-mo force-pushed the param_coding branch from 3e644b4 to 5ddc3f9 Compare November 27, 2019 16:35

lbdreyer reviewed Nov 27, 2019

View reviewed changes

iris_grib/tests/unit/save_rules/test_set_discipline_and_parameter.py Outdated Show resolved Hide resolved

pp-mo force-pushed the param_coding branch 2 times, most recently from 9765acc to 7ee59c5 Compare November 27, 2019 17:09

lbdreyer reviewed Nov 29, 2019

View reviewed changes

iris_grib/tests/unit/save_rules/test_set_discipline_and_parameter.py Outdated Show resolved Hide resolved

lbdreyer reviewed Nov 29, 2019

View reviewed changes

lbdreyer requested changes Nov 29, 2019

View reviewed changes

lbdreyer reviewed Dec 2, 2019

View reviewed changes

iris_grib/_load_convert.py Outdated Show resolved Hide resolved

pp-mo added 4 commits December 2, 2019 12:14

Support unknown phenomena with a 'GRIB_CODING' attribute; tests passing.

767392a

Tests for save behaviour and GribCode api; revised coding and error m…

73f9ab1

…essages.

Make load-unknown test more sensible.

fbcdf60

Style fixes.

d532422

pp-mo and others added 5 commits December 2, 2019 12:14

Make GRIB_CODING usage more stable, and more like STASH.

fb6b2ce

Fix oldstyle header.

be569fe

Tidy testcube usage + add extra test.

7cf98ed

Python 3 : import mock from unittest

dfb726e

Co-Authored-By: lbdreyer <[email protected]>

Purge Python2 legacy.

6f62dd6

pp-mo force-pushed the param_coding branch from 3ae39bb to 6f62dd6 Compare December 2, 2019 12:26

pp-mo added 2 commits December 2, 2019 12:46

Review changes: revised classname, attribute name and print format.

601ee46

Test a round-trip: gribcode -- string -- gribcode.

d2720d4

lbdreyer reviewed Dec 2, 2019

View reviewed changes

Remove six; fix class name.

efcbfdd

Remove rogue import of 'six'.

ff2bc89

lbdreyer merged commit ddcac75 into SciTools:master Dec 4, 2019

pp-mo mentioned this pull request Jun 15, 2020

Provide format-specific phenomenon identity for load and save #153

Closed

pp-mo deleted the param_coding branch June 16, 2020 13:03

pp-mo mentioned this pull request Oct 10, 2020

iris grib2 loading not writting parameter codes to cube SciTools/iris#563

Closed

		return _CF_GRIB2_TABLE[(standard_name, long_name)]


		class GribCode(namedtuple('GribCode',

Label unknown phenomena with a 'GRIB_CODING' attribute #156

Label unknown phenomena with a 'GRIB_CODING' attribute #156

Uh oh!

Conversation

pp-mo commented Sep 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Sep 6, 2019

Uh oh!

coveralls commented Sep 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pp-mo commented Sep 13, 2019

Uh oh!

pp-mo commented Sep 13, 2019

Uh oh!

pp-mo commented Sep 13, 2019

Uh oh!

pp-mo commented Sep 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pp-mo commented Nov 27, 2019

Uh oh!

Uh oh!

pp-mo commented Nov 27, 2019

Uh oh!

Uh oh!

lbdreyer Nov 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lbdreyer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lbdreyer commented Dec 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pp-mo commented Dec 2, 2019

Uh oh!

pp-mo commented Dec 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pp-mo Dec 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pp-mo commented Dec 2, 2019

Uh oh!

lbdreyer commented Dec 3, 2019

Uh oh!

pp-mo commented Dec 3, 2019

pp-mo commented Sep 6, 2019 •

edited

Loading

coveralls commented Sep 6, 2019 •

edited

Loading

pp-mo commented Sep 16, 2019 •

edited

Loading

lbdreyer Nov 29, 2019 •

edited

Loading

lbdreyer commented Dec 2, 2019 •

edited

Loading

pp-mo Dec 2, 2019 •

edited

Loading