From a254d3965260ad9ed4c50fe66235fe3411876182 Mon Sep 17 00:00:00 2001 From: Patrick Peglar Date: Mon, 26 Sep 2022 13:47:37 +0100 Subject: [PATCH 1/2] Add 'global_attributes' field into CubeMetadata. --- lib/iris/common/metadata.py | 51 ++++++-- .../unit/common/metadata/test_CubeMetadata.py | 114 ++++++++++++------ 2 files changed, 116 insertions(+), 49 deletions(-) diff --git a/lib/iris/common/metadata.py b/lib/iris/common/metadata.py index 8ec39bb4b1..a63af31291 100644 --- a/lib/iris/common/metadata.py +++ b/lib/iris/common/metadata.py @@ -595,7 +595,7 @@ def _difference_strict_attributes(left, right): def _is_attributes(field, left, right): """Determine whether we have two 'attributes' dictionaries.""" return ( - field == "attributes" + field in ("attributes", "global_attributes") and isinstance(left, Mapping) and isinstance(right, Mapping) ) @@ -1081,7 +1081,7 @@ class CubeMetadata(BaseMetadata): """ - _members = "cell_methods" + _members = ("cell_methods", "global_attributes") __slots__ = () @@ -1101,7 +1101,11 @@ def __lt__(self, other): def _sort_key(item): keys = [] for field in item._fields: - if field not in ("attributes", "cell_methods"): + if field not in ( + "attributes", + "global_attributes", + "cell_methods", + ): value = getattr(item, field) keys.extend((value is not None, value)) return tuple(keys) @@ -1121,16 +1125,30 @@ def _combine_lenient(self, other): A list of combined metadata member values. """ + # Perform lenient combination of parent (BaseMetadata) members. + result = super()._combine_lenient(other) # Perform "strict" combination for "cell_methods". value = ( self.cell_methods if self.cell_methods == other.cell_methods else None ) - # Perform lenient combination of the other parent members. - result = super()._combine_lenient(other) result.append(value) - + # Perform lenient combination for "global_attributes" + left = self.global_attributes + right = other.global_attributes + value = None + if self._is_attributes("global_attributes", left, right): + value = self._combine_lenient_attributes(left, right) + else: + # Default logic is the general fallback for member comparison + if left == right: + value = left + elif left is None: + value = right + elif right is None: + value = left + result.append(value) return result def _compare_lenient(self, other): @@ -1149,8 +1167,13 @@ def _compare_lenient(self, other): # Perform "strict" comparison for "cell_methods". result = self.cell_methods == other.cell_methods if result: + # Perform lenient comparison of global_attributes. + result = self._compare_lenient_attributes( + self.global_attributes, other.global_attributes + ) + if result: + # Compare BaseMetadata aspects. result = super()._compare_lenient(other) - return result def _difference_lenient(self, other): @@ -1166,16 +1189,20 @@ def _difference_lenient(self, other): A list of difference metadata member values. """ - # Perform "strict" difference for "cell_methods". - value = ( + # Perform lenient difference of the other parent members. + result = super()._difference_lenient(other) + # Calculate + add "strict" difference for "cell_methods". + cms_value = ( None if self.cell_methods == other.cell_methods else (self.cell_methods, other.cell_methods) ) - # Perform lenient difference of the other parent members. - result = super()._difference_lenient(other) - result.append(value) + # Calculate + add "lenient" difference for "global_attributes". + gattrs_value = self._difference_lenient_attributes( + self.global_attributes, other.global_attributes + ) + result.extend([cms_value, gattrs_value]) return result @property diff --git a/lib/iris/tests/unit/common/metadata/test_CubeMetadata.py b/lib/iris/tests/unit/common/metadata/test_CubeMetadata.py index ac47735393..4551279807 100644 --- a/lib/iris/tests/unit/common/metadata/test_CubeMetadata.py +++ b/lib/iris/tests/unit/common/metadata/test_CubeMetadata.py @@ -27,6 +27,7 @@ def _make_metadata( long_name=None, var_name=None, attributes=None, + global_attributes=None, force_mapping=True, ): if force_mapping: @@ -42,6 +43,7 @@ def _make_metadata( units=None, attributes=attributes, cell_methods=None, + global_attributes=global_attributes, ) @@ -53,6 +55,7 @@ def setUp(self): self.units = mock.sentinel.units self.attributes = mock.sentinel.attributes self.cell_methods = mock.sentinel.cell_methods + self.global_attributes = mock.sentinel.global_attributes self.cls = CubeMetadata def test_repr(self): @@ -63,10 +66,11 @@ def test_repr(self): units=self.units, attributes=self.attributes, cell_methods=self.cell_methods, + global_attributes=self.global_attributes, ) fmt = ( "CubeMetadata(standard_name={!r}, long_name={!r}, var_name={!r}, " - "units={!r}, attributes={!r}, cell_methods={!r})" + "units={!r}, attributes={!r}, cell_methods={!r}, global_attributes={!r})" ) expected = fmt.format( self.standard_name, @@ -75,6 +79,7 @@ def test_repr(self): self.units, self.attributes, self.cell_methods, + self.global_attributes, ) self.assertEqual(expected, repr(metadata)) @@ -86,6 +91,7 @@ def test__fields(self): "units", "attributes", "cell_methods", + "global_attributes", ) self.assertEqual(self.cls._fields, expected) @@ -94,15 +100,23 @@ def test_bases(self): @pytest.fixture(params=CubeMetadata._fields) +# Fixture to parametrise tests over all CubeMetadata fields. def fieldname(request): return request.param @pytest.fixture(params=["strict", "lenient"]) +# Fixture to parametrise tests over strict/lenient behaviour. def op_leniency(request): return request.param +@pytest.fixture(params=["attributes", "global_attributes"]) +# Fixture to parametrise atrribute tests over global/local attributes fields. +def attributes_fieldname(request): + return request.param + + class Test___eq__: @pytest.fixture(autouse=True) def setup(self): @@ -114,6 +128,8 @@ def setup(self): # Must be a mapping. attributes=dict(), cell_methods=sentinel.cell_methods, + # Must be a mapping. + global_attributes=dict(), ) # Setup another values tuple with all-distinct content objects. self.rvalues = deepcopy(self.lvalues) @@ -157,7 +173,7 @@ def test_op_same(self, op_leniency): def test_op_different__none(self, fieldname, op_leniency): # One side has field=value, and the other field=None, both strict + lenient. - if fieldname == "attributes": + if fieldname in ("attributes", "global_attributes"): # Must be a dict, cannot be None. pytest.skip() else: @@ -186,7 +202,7 @@ def test_op_different__none(self, fieldname, op_leniency): def test_op_different__value(self, fieldname, op_leniency): # Compare when a given field value is changed, both strict + lenient. - if fieldname == "attributes": + if fieldname in ("attributes", "global_attributes"): # Dicts have more possibilities: handled separately. pytest.skip() else: @@ -218,11 +234,13 @@ def test_op_different__value(self, fieldname, op_leniency): assert lmetadata.__eq__(rmetadata) == expect_success assert rmetadata.__eq__(lmetadata) == expect_success - def test_op_different__attribute_extra(self, op_leniency): + def test_op_different__attribute_extra( + self, attributes_fieldname, op_leniency + ): # Check when one set of attributes has an extra entry. is_lenient = op_leniency == "lenient" lmetadata = self.cls(**self.lvalues) - self.rvalues["attributes"]["_extra_"] = 1 + self.rvalues[attributes_fieldname]["_extra_"] = 1 rmetadata = self.cls(**self.rvalues) # This counts as equal *only* in the lenient case. expect_success = is_lenient @@ -233,11 +251,13 @@ def test_op_different__attribute_extra(self, op_leniency): assert lmetadata.__eq__(rmetadata) == expect_success assert rmetadata.__eq__(lmetadata) == expect_success - def test_op_different__attribute_value(self, op_leniency): + def test_op_different__attribute_value( + self, attributes_fieldname, op_leniency + ): # lhs and rhs have different values for an attribute, both strict + lenient. is_lenient = op_leniency == "lenient" - self.lvalues["attributes"]["_extra_"] = mock.sentinel.value1 - self.rvalues["attributes"]["_extra_"] = mock.sentinel.value2 + self.lvalues[attributes_fieldname]["_extra_"] = mock.sentinel.value1 + self.rvalues[attributes_fieldname]["_extra_"] = mock.sentinel.value2 lmetadata = self.cls(**self.lvalues) rmetadata = self.cls(**self.rvalues) with mock.patch( @@ -251,10 +271,12 @@ def test_op_different__attribute_value(self, op_leniency): class Test___lt__(tests.IrisTest): def setUp(self): self.cls = CubeMetadata - self.one = self.cls(1, 1, 1, 1, 1, 1) - self.two = self.cls(1, 1, 1, 2, 1, 1) - self.none = self.cls(1, 1, 1, None, 1, 1) - self.attributes_cm = self.cls(1, 1, 1, 1, 10, 10) + self.one = self.cls(1, 1, 1, 1, 1, 1, 1) + self.two = self.cls(1, 1, 1, 2, 1, 1, 1) + self.none = self.cls(1, 1, 1, None, 1, 1, 1) + self.attributes = self.cls(1, 1, 1, 1, 10, 1, 1) + self.cm = self.cls(1, 1, 1, 1, 1, 10, 1) + self.globalattributes = self.cls(1, 1, 1, 1, 1, 1, 10) def test__ascending_lt(self): result = self.one < self.two @@ -272,11 +294,15 @@ def test__none_lhs_operand(self): result = self.none < self.one self.assertTrue(result) - def test__ignore_attributes_cell_methods(self): - result = self.one < self.attributes_cm - self.assertFalse(result) - result = self.attributes_cm < self.one - self.assertFalse(result) + def test__ignore_attributes_cellmethods_globalattributes(self): + # None of these things is comparable : *all* comparisons fail when any of + # these differs between lhs + rhs. + self.assertFalse(self.one < self.cm) + self.assertFalse(self.cm < self.one) + self.assertFalse(self.one < self.attributes) + self.assertFalse(self.attributes < self.one) + self.assertFalse(self.one < self.globalattributes) + self.assertFalse(self.globalattributes < self.one) class Test_combine: @@ -289,6 +315,7 @@ def setup(self): units=sentinel.units, attributes=sentinel.attributes, cell_methods=sentinel.cell_methods, + global_attributes=sentinel.global_attributes, ) # Get a second copy with all-new objects. self.rvalues = deepcopy(self.lvalues) @@ -344,7 +371,7 @@ def test_op_same(self, op_leniency): def test_op_different__none(self, fieldname, op_leniency): # One side has field=value, and the other field=None, both strict + lenient. - if fieldname == "attributes": + if fieldname in ("attributes", "global_attributes"): # Can't be None : Tested separately pytest.skip() @@ -405,13 +432,19 @@ def test_op_different__value(self, fieldname, op_leniency): assert lmetadata.combine(rmetadata)._asdict() == expected assert rmetadata.combine(lmetadata)._asdict() == expected - def test_op_different__attribute_extra(self, op_leniency): + def test_op_different__attribute_extra( + self, attributes_fieldname, op_leniency + ): # One field has an extra attribute, both strict + lenient. is_lenient = op_leniency == "lenient" - self.lvalues["attributes"] = {"_a_common_": mock.sentinel.dummy} - self.rvalues["attributes"] = self.lvalues["attributes"].copy() - self.rvalues["attributes"]["_extra_"] = mock.sentinel.testvalue + self.lvalues[attributes_fieldname] = { + "_a_common_": mock.sentinel.dummy + } + self.rvalues[attributes_fieldname] = self.lvalues[ + attributes_fieldname + ].copy() + self.rvalues[attributes_fieldname]["_extra_"] = mock.sentinel.testvalue lmetadata = self.cls(**self.lvalues) rmetadata = self.cls(**self.rvalues) @@ -429,15 +462,17 @@ def test_op_different__attribute_extra(self, op_leniency): assert lmetadata.combine(rmetadata)._asdict() == expected assert rmetadata.combine(lmetadata)._asdict() == expected - def test_op_different__attribute_value(self, op_leniency): + def test_op_different__attribute_value( + self, attributes_fieldname, op_leniency + ): # lhs and rhs have different values for an attribute, both strict + lenient. is_lenient = op_leniency == "lenient" - self.lvalues["attributes"] = { + self.lvalues[attributes_fieldname] = { "_a_common_": self.dummy, "_b_common_": mock.sentinel.value1, } - self.lvalues["attributes"] = { + self.lvalues[attributes_fieldname] = { "_a_common_": self.dummy, "_b_common_": mock.sentinel.value2, } @@ -447,7 +482,7 @@ def test_op_different__attribute_value(self, op_leniency): # Result has entirely EMPTY attributes (whether strict or lenient). # TODO: is this maybe a mistake of the existing implementation ? expected = self.lvalues.copy() - expected["attributes"] = None + expected[attributes_fieldname] = None with mock.patch( "iris.common.metadata._LENIENT", return_value=is_lenient @@ -467,6 +502,7 @@ def setup(self): units=sentinel.units, attributes=dict(), # MUST be a dict cell_methods=sentinel.cell_methods, + global_attributes=dict(), # MUST be a dict ) # Make a copy with all-different objects in it. self.rvalues = deepcopy(self.lvalues) @@ -519,7 +555,7 @@ def test_op_same(self, op_leniency): def test_op_different__none(self, fieldname, op_leniency): # One side has field=value, and the other field=None, both strict + lenient. - if fieldname in ("attributes",): + if fieldname in ("attributes", "global_attributes"): # These cannot properly be set to 'None'. Tested elsewhere. pytest.skip() @@ -585,22 +621,24 @@ def test_op_different__value(self, fieldname, op_leniency): assert lmetadata.difference(rmetadata)._asdict() == ldiff_metadata assert rmetadata.difference(lmetadata)._asdict() == rdiff_metadata - def test_op_different__attribute_extra(self, op_leniency): + def test_op_different__attribute_extra( + self, attributes_fieldname, op_leniency + ): # One field has an extra attribute, both strict + lenient. is_lenient = op_leniency == "lenient" - self.lvalues["attributes"] = {"_a_common_": self.dummy} + self.lvalues[attributes_fieldname] = {"_a_common_": self.dummy} lmetadata = self.cls(**self.lvalues) rvalues = deepcopy(self.lvalues) - rvalues["attributes"]["_b_extra_"] = mock.sentinel.extra + rvalues[attributes_fieldname]["_b_extra_"] = mock.sentinel.extra rmetadata = self.cls(**rvalues) if not is_lenient: # In this case, attributes returns a "difference dictionary" diffentry = tuple([{}, {"_b_extra_": mock.sentinel.extra}]) lexpected = self.none._asdict() - lexpected["attributes"] = diffentry + lexpected[attributes_fieldname] = diffentry rexpected = lexpected.copy() - rexpected["attributes"] = diffentry[::-1] + rexpected[attributes_fieldname] = diffentry[::-1] with mock.patch( "iris.common.metadata._LENIENT", return_value=is_lenient @@ -614,15 +652,17 @@ def test_op_different__attribute_extra(self, op_leniency): assert lmetadata.difference(rmetadata)._asdict() == lexpected assert rmetadata.difference(lmetadata)._asdict() == rexpected - def test_op_different__attribute_value(self, op_leniency): + def test_op_different__attribute_value( + self, attributes_fieldname, op_leniency + ): # lhs and rhs have different values for an attribute, both strict + lenient. is_lenient = op_leniency == "lenient" - self.lvalues["attributes"] = { + self.lvalues[attributes_fieldname] = { "_a_common_": self.dummy, "_b_extra_": mock.sentinel.value1, } lmetadata = self.cls(**self.lvalues) - self.rvalues["attributes"] = { + self.rvalues[attributes_fieldname] = { "_a_common_": self.dummy, "_b_extra_": mock.sentinel.value2, } @@ -636,9 +676,9 @@ def test_op_different__attribute_value(self, op_leniency): ] ) lexpected = self.none._asdict() - lexpected["attributes"] = diffentry + lexpected[attributes_fieldname] = diffentry rexpected = lexpected.copy() - rexpected["attributes"] = diffentry[::-1] + rexpected[attributes_fieldname] = diffentry[::-1] with mock.patch( "iris.common.metadata._LENIENT", return_value=is_lenient From 6cc463056c91fc066da7a407e7f3c4d790214b98 Mon Sep 17 00:00:00 2001 From: Patrick Peglar Date: Fri, 28 Oct 2022 12:01:56 +0100 Subject: [PATCH 2/2] Temporary fixes for major test breakers. --- lib/iris/_concatenate.py | 6 ++++++ lib/iris/_merge.py | 9 +++++++++ lib/iris/common/mixin.py | 6 +++++- lib/iris/experimental/stratify.py | 5 ++++- lib/iris/tests/test_cdm.py | 13 ++++++++++--- .../tests/unit/common/mixin/test_CFVariableMixin.py | 2 ++ lib/iris/tests/unit/common/resolve/test_Resolve.py | 1 + 7 files changed, 37 insertions(+), 5 deletions(-) diff --git a/lib/iris/_concatenate.py b/lib/iris/_concatenate.py index 5debc452ee..fef2ba57a7 100644 --- a/lib/iris/_concatenate.py +++ b/lib/iris/_concatenate.py @@ -784,6 +784,12 @@ def concatenate(self): # Build the new cube. kwargs = cube_signature.defn._asdict() + # Hack for now just to make this work, since we added a "global_attributes" + # field to CubeMetadata, which we are not yet using, but we have no matching + # keyword in the Cube constructor call. + kwargs.pop("global_attributes") + # TODO-splitattrs: in future we will provide some way of passing separate + # local+global attributes into the Cube constructor. Fix this properly then. cube = iris.cube.Cube( data, dim_coords_and_dims=dim_coords_and_dims, diff --git a/lib/iris/_merge.py b/lib/iris/_merge.py index bc12080523..2bc27e1981 100644 --- a/lib/iris/_merge.py +++ b/lib/iris/_merge.py @@ -391,6 +391,9 @@ def _defn_msgs(self, other_defn): ) ) if self_defn.attributes != other_defn.attributes: + # TODO-splitattrs: this needs to change to handle both global + local + # attributes, when they are in active use. We probably *ought* to be using + # metadata.difference for this. diff_keys = set(self_defn.attributes.keys()) ^ set( other_defn.attributes.keys() ) @@ -1594,6 +1597,12 @@ def _get_cube(self, data): for coord, dims in self._aux_coords_and_dims ] kwargs = dict(zip(CubeMetadata._fields, signature.defn)) + # Hack for now just to make this work, since we added a "global_attributes" + # field to CubeMetadata, which we are not yet using, but we have no matching + # keyword in the Cube constructor call. + kwargs.pop("global_attributes") + # TODO-splitattrs: in future we will provide some way of passing separate + # local+global attributes into the Cube constructor. Fix this properly then. cms_and_dims = [ (deepcopy(cm), dims) for cm, dims in self._cell_measures_and_dims diff --git a/lib/iris/common/mixin.py b/lib/iris/common/mixin.py index 4c19dd756b..17a9f13a3b 100644 --- a/lib/iris/common/mixin.py +++ b/lib/iris/common/mixin.py @@ -242,4 +242,8 @@ def metadata(self, metadata): # Ensure to always set state through the individual mixin/container # setter functions. - setattr(self, field, value) + # Since we don't have a 'global_attributes' cube property, we need to avoid + # setting a property that doesn't exist. + # TODO - splitattrs: fix this properly later. + if hasattr(self, field): + setattr(self, field, value) diff --git a/lib/iris/experimental/stratify.py b/lib/iris/experimental/stratify.py index a7cfbf6876..ed5d37c5d3 100644 --- a/lib/iris/experimental/stratify.py +++ b/lib/iris/experimental/stratify.py @@ -172,7 +172,10 @@ def relevel(cube, src_levels, tgt_levels, axis=None, interpolator=None): new_data = interpolator(tgt_levels, src_data, cube_data, axis=axis) # Create a result cube with the correct shape and metadata. - result = Cube(new_data, **cube.copy().metadata._asdict()) + # TODO-splitattrs: fix this + kwargs = cube.metadata._asdict() + kwargs.pop("global_attributes") + result = Cube(new_data, **kwargs) # Copy across non z-dimension coordinates from the source cube # to the result cube. diff --git a/lib/iris/tests/test_cdm.py b/lib/iris/tests/test_cdm.py index 0615dc39bf..00ea967c2f 100644 --- a/lib/iris/tests/test_cdm.py +++ b/lib/iris/tests/test_cdm.py @@ -950,7 +950,15 @@ def test_metadata_nop(self): self.assertEqual(self.t.cell_methods, ()) def test_metadata_tuple(self): - metadata = ("air_pressure", "foo", "bar", "", {"random": "12"}, ()) + metadata = ( + "air_pressure", + "foo", + "bar", + "", + {"random": "12"}, + (), + {"extra": 4}, + ) self.t.metadata = metadata self.assertEqual(self.t.standard_name, "air_pressure") self.assertEqual(self.t.long_name, "foo") @@ -989,7 +997,7 @@ class Metadata: metadata.units = "" metadata.attributes = {"random": "12"} metadata.cell_methods = () - metadata.cell_measures_and_dims = [] + metadata.global_attributes = {"extra": 4} self.t.metadata = metadata self.assertEqual(self.t.standard_name, "air_pressure") self.assertEqual(self.t.long_name, "foo") @@ -998,7 +1006,6 @@ class Metadata: self.assertEqual(self.t.attributes, metadata.attributes) self.assertIsNot(self.t.attributes, metadata.attributes) self.assertEqual(self.t.cell_methods, ()) - self.assertEqual(self.t._cell_measures_and_dims, []) def test_metadata_fail(self): with self.assertRaises(TypeError): diff --git a/lib/iris/tests/unit/common/mixin/test_CFVariableMixin.py b/lib/iris/tests/unit/common/mixin/test_CFVariableMixin.py index 88a88be567..f87767dead 100644 --- a/lib/iris/tests/unit/common/mixin/test_CFVariableMixin.py +++ b/lib/iris/tests/unit/common/mixin/test_CFVariableMixin.py @@ -314,10 +314,12 @@ def test_class_coordmetadata(self): def test_class_cubemetadata(self): self.args["cell_methods"] = None + self.args["global_attributes"] = None metadata = CubeMetadata(**self.args) self.item.metadata = metadata expected = metadata._asdict() del expected["cell_methods"] + del expected["global_attributes"] self.assertEqual(self.item._metadata_manager.values, expected) self.assertIsNot( self.item._metadata_manager.attributes, metadata.attributes diff --git a/lib/iris/tests/unit/common/resolve/test_Resolve.py b/lib/iris/tests/unit/common/resolve/test_Resolve.py index 840f65db01..d335821276 100644 --- a/lib/iris/tests/unit/common/resolve/test_Resolve.py +++ b/lib/iris/tests/unit/common/resolve/test_Resolve.py @@ -4466,6 +4466,7 @@ def setUp(self): units=Unit("K"), attributes={}, cell_methods=(), + global_attributes=None, # TODO-splitattrs: should be {}, when implemented ) lhs_cube = Cube(self.data) lhs_cube.metadata = self.cube_metadata