Support Standard Name Modifers (not units!) #3393

lbdreyer · 2019-09-09T12:23:28Z

CF conventions support a set of standard_name modifiers. See 3.3. Standard Name for more details, specifically:

A standard name is associated with a variable via the attribute standard_name which takes a string value comprised of a standard name optionally followed by one or more blanks and a standard name modifier (a string value from Appendix C, Standard Name Modifiers).

This PR modifies the CFVariableMixin standard_name setter (and the pyke rules) to support standard name modifiers.

I found there wasn't much testing of cube/coord names so I have split this PR into two commits:

The first commit has no functional changes. It just adds testing of the pyke rules to show that they don't break later when I add the functional changes
The second commit adds the changes to include the standard_name modifiers

Note that standard name modifiers have associated units (See CF Appendix C) however I have not included that in this PR as I am only modifying the standard_name setter which at the moment doesn't have any requirement on the unit

lbdreyer · 2019-09-09T12:39:08Z

Ping @bjlittle !

bjlittle

@lbdreyer Nice one, over to you... 😄

bjlittle · 2019-09-12T14:53:37Z

lib/iris/fileformats/cf.py

+# Supported standard name modifiers. Ref: [CF] Appendix C.
+STD_NAME_MODIFIERS = ['detection_minimum', 'number_of_observations',
+                      'standard_error', 'status_flag']
+


@lbdreyer It's kinda weird to define constants here that aren't used in this module. Rather they're used in _cube_coord_common.py, and specifically in only one place...

I see the intent, but would you consider binding their use closer to their definition? i.e., actually define STD_NAME_MODIFIERS as a variable within the iris._cube_coord_common.get_valid_standard_name function.

lib/iris/_cube_coord_common.py

bjlittle · 2019-09-17T09:27:46Z

lib/iris/_cube_coord_common.py

            self._standard_name = name
        else:
-            raise ValueError('%r is not a valid standard_name' % name)
+            self._standard_name = get_valid_standard_name(name)


You could change get_valid_standard_name so that if None is passed in, then None is passed out. That way any code that calls get_valid_standard_name doesn't need to explicitly handle the None case (as above).

So here we would simply have:

@standard_name.setter def standard_name(self, name): self._standard_name = get_valid_standard_name(name)

bjlittle · 2019-09-17T10:50:22Z

lib/iris/_cube_coord_common.py

+                    standard_name_modifier not in
+                    iris.fileformats.cf.STD_NAME_MODIFIERS):
+                raise ValueError(error_msg)
+        return name


Consider pulling the return name out from within the embedded if else and make it the last statement of the function i.e.,

def get_valid_standard_name(name): ... if ... ... else: ... return name

Yeah I agree, it would be nicer. I just couldn't quite think through how to do that, but I will give it another look!

@lbdreyer I think that all you need to do is simply move the return name outside the if ... else ..., as your not changing the contents of name, the output of the function is just the same as the input.

If the input std_name (from name) is valid, then it will pass through the if ..., and then if the standard_name_modifier is valid it will simply drop through and return name. On the otherhand, if the modifier is not valid an exception is raised and the return name is never be reached.

If the input name is invalid, then it will pass through to the else ... where an exception is raised and the return name will never be reached.

Should just work, right? Check my logic is right, though...

bjlittle · 2019-09-17T13:49:40Z

lib/iris/_cube_coord_common.py

+    # Standard names are optionally followed by a standard name
+    # modifier, separated by one or more blank spaces
+    name_parts = name.split(' ')
+    std_name, std_name_extension = name_parts[0], name_parts[1:]


Using a (compiled) regular expression would allow you to easily pluck out the standard name and modifier, and avoid dealing with a std_name_extension that is a list.

lib/iris/fileformats/_pyke_rules/fc_rules_cf.krb

bjlittle · 2019-09-17T14:09:25Z

lib/iris/tests/integration/test_netcdf.py

+        with self.temp_filename(suffix='.nc') as fout:
+            iris.save(cube, fout)
+            detection_limit_cube = iris.load_cube(fout)
+            self.assertEqual(detection_limit_cube.standard_name, standard_name)


bjlittle · 2019-09-17T14:14:09Z

lib/iris/tests/unit/cube_coord_common/test_CFVariableMixin.py

+class Test_standard_name__setter(tests.IrisTest):
+    def test_valid_standard_name(self):
+        cf_var = CFVariableMixin()
+        cf_var.standard_name = 'air_temperature'


Shouldn't you then self.assertEqual(cf_var.standard_name, 'air_temperature') to ensure that we get back what we set?

I guess my concern with that was that we are then testing the standard_name getter and setter at the same time, which doesn't feel very unit-testy but I guess we should test not just that setting the standard doesn't raise errors, but also that it is done correctly and it's hard to do that in isolation from testing the getter.

bjlittle · 2019-09-17T14:14:57Z

lib/iris/tests/unit/cube_coord_common/test_CFVariableMixin.py

+
+    def test_none_standard_name(self):
+        cf_var = CFVariableMixin()
+        cf_var.standard_name = None


Also, include a self.assertIsNone(cf_var.standard_name)?

bjlittle · 2019-09-17T14:32:30Z

...is/tests/unit/fileformats/pyke_rules/compiled_krb/fc_rules_cf_fc/test_build_cube_metadata.py

-        engine = mock.Mock(
-            cube=Cube([23]),
-            cf_var=cf_var)
+    return engine


Wondered why you pulled this out of TestInvalidGlobalAttributes, but you're also using it in the new test class TestCubeName... nice 👍

bjlittle · 2019-09-17T14:39:26Z

@lbdreyer What's the strategy for checking the units of a standard_name with a standard name modifier?

Separate PR?

bjlittle · 2019-09-17T14:45:35Z

lib/iris/tests/unit/fileformats/pyke_rules/compiled_krb/fc_rules_cf_fc/test_get_names.py

+        inp = ('latitude_coord', 'lat_long_name', 'lat_var_name', 'latitude')
+        exp = ('latitude', 'lat_long_name', 'lat_var_name',
+               {'invalid_standard_name': 'latitude_coord'})
+        self.check_names(inp, exp)


@lbdreyer Whoa... nice test coverage 🎉

…d_name modifiers is added

lbdreyer · 2019-09-20T11:22:13Z

@lbdreyer What's the strategy for checking the units of a standard_name with a standard name modifier?

Separate PR?

I have raised this in a separate issue: #3408

lbdreyer · 2019-09-20T13:47:34Z

@bjlittle I believe I have addressed your review comments in the third commit "review actions"

Note: I had to rebase the first two commits due a conflict that was caused when #3399 was merged, but those commits should be the same as they were before.

bjlittle · 2019-09-24T06:34:52Z

@lbdreyer Awesome, thanks! 🎉

lbdreyer requested a review from bjlittle September 9, 2019 12:23

lbdreyer added this to the v2.3.0 milestone Sep 9, 2019

bjlittle self-assigned this Sep 9, 2019

bjlittle added Feature: NetCDF + CF-conventions Release: Minor Type: Enhancement labels Sep 9, 2019

bjlittle requested changes Sep 17, 2019

View reviewed changes

bjlittle reviewed Sep 17, 2019

View reviewed changes

lbdreyer mentioned this pull request Sep 20, 2019

Decide what to do about standard name modifiers units #3408

Closed

lbdreyer added 2 commits September 20, 2019 12:12

Add preliminary tests to pyke rules to test the naming before standar…

5090d43

…d_name modifiers is added

Add support for standard name modifiers in name setter

e9baffd

lbdreyer force-pushed the std_name_modifiers branch from b677563 to 8e30737 Compare September 20, 2019 12:33

review actions

101dc5c

lbdreyer force-pushed the std_name_modifiers branch from 8e30737 to 101dc5c Compare September 20, 2019 13:42

bjlittle approved these changes Sep 24, 2019

View reviewed changes

bjlittle merged commit 6bb3fa4 into SciTools:master Sep 24, 2019

lbdreyer mentioned this pull request Sep 27, 2019

[PI] Quality Flag Handling #3358

Closed

5 tasks

pp-mo mentioned this pull request Mar 5, 2020

Valid standard_name (present in STD_NAMES dict) is still rejected because of formatting maflormation #3677

Closed

lbdreyer deleted the std_name_modifiers branch June 23, 2021 15:31

tjtg mentioned this pull request Aug 13, 2021

ResolveWindComponents auxiliary coordinate errors with Iris >2.2 metoppv/improver#1534

Closed

Support Standard Name Modifers (not units!) #3393

Support Standard Name Modifers (not units!) #3393

Uh oh!

Conversation

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

Uh oh!

Uh oh!

lbdreyer commented Sep 9, 2019

Uh oh!

bjlittle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lbdreyer Sep 18, 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!

Choose a reason for hiding this comment

Uh oh!

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!

bjlittle commented Sep 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lbdreyer commented Sep 20, 2019

Uh oh!

lbdreyer commented Sep 20, 2019

Uh oh!

bjlittle commented Sep 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lbdreyer commented Sep 9, 2019 •

edited

Loading

lbdreyer Sep 18, 2019 •

edited

Loading