[FB] [PI-3478] Common metadata API #3583

bjlittle · 2019-12-05T11:11:45Z

This PR introduces a common metadata API for cubes, coordinates and auxiliary factories, and also addresses a lot of connective tissue technical debt. This PR underpins the forthcoming changes for cube arithmetic.

Essentially, this PR proposes a metadata interface, which provides a unified, consistent treatment of metadata within iris:

the _as_defn interface has been replaced with the metadata interface
metadata state now lives with the metadata behaviour within a MetadataManager class:
- a factory function creates MetadataManager classes at runtime, see iris.common.metadata.MetadataManagerFactory
- the factory function requires one of the following namedtuple metadata subclasses, which specifies the behaviour and state associated with the runtime MetadataManager:
  - iris.common.metadata.AncillaryVariableMetadata
  - iris.common.metadata.BaseMetadata
  - iris.common.metadata.CellMeasureMetadata
  - iris.common.metadata.CoordMetadata
the metadata interface has been added to:
- iris.aux_factory.AuxCoordFactory
- iris.coords._DimensionalMetadata
- iris.coords.AncillaryVariable
- iris.coords.CellMeasure
- iris.coords.Coord
- iris.cube.Cube
the CFVariableMixin methods have been rationalised
- common member setter and getters now use the metadata interface
- added a new metadata member setter and getter
  - metadata may be set either with:
    - an iterable
    - a dictionary
    - a relevant metadata namedtuple subclass, see iris.common.metadata
there is a new iris.common module, for common, non-core iris behaviour:
- the _cube_coord_common has been rationalised here, see iris.common.mixin
- the metadata has been rationalised here, see iris.common.metadata
the mixin name method behaviour is now appropriate and specific to its container i.e., the STASH attribute used by name is now only applicable to an iris.cube.Cube
the recently introduced mixin names property behaviour is now private and specific to an iris.cube.Cube
added new unit test coverage for LimitedAttributeDict, which has always been missing
For iris.aux_factory.AuxCoordFactory:
- the units are no longer a read-only property, but part of the containers metadata
- climatological is now a read-only property of False

Still to do:

Test coverage for new iris.common.metadata.MetadataFactory
Test coverage for new iris.common.metadata._BaseMeta
Test coverage for existing iris.common.mixin.LimitedAttributeDict

bjlittle · 2019-12-06T07:39:25Z

lib/iris/common/metadata.py

+    Meta-class to support the convenience of creating a namedtuple from
+    names/members of the metadata class hierarchy.
+
+    """


Reference:

https://docs.python.org/3/reference/datamodel.html?highlight=namespace#object.__new__

https://docs.python.org/3/reference/datamodel.html?highlight=namespace#metaclasses

https://docs.python.org/3/reference/datamodel.html?highlight=namespace#creating-the-class-object

bjlittle · 2019-12-06T07:40:15Z

lib/iris/common/metadata.py

+        "attributes",
+    )
+
+    __slots__ = ()


Reference:

https://docs.python.org/3/reference/datamodel.html?highlight=__slots__#object.__slots__

bjlittle · 2019-12-06T07:41:19Z

lib/iris/common/metadata.py

+        return (standard_name, long_name, var_name, stash_name)
+
+
+def MetadataFactory(cls, **kwargs):


Note that, this is a function, not a class...

bjlittle · 2019-12-06T07:42:05Z

lib/iris/common/metadata.py

+            match = self.values == other.values
+        return match
+
+    def __getstate__(self):


Reference:

https://docs.python.org/3/library/pickle.html?highlight=__getstate__#object.__getstate__

bjlittle · 2019-12-06T07:42:25Z

lib/iris/common/metadata.py

+            match = not match
+        return match
+
+    def __reduce__(self):


Reference:

https://docs.python.org/3/library/pickle.html?highlight=__getstate__#object.__reduce__

bjlittle · 2019-12-06T07:43:15Z

lib/iris/common/metadata.py

+        )
+        return "{}({})".format(self.__class__.__name__, args)
+
+    def __setstate__(self, state):


Reference:

https://docs.python.org/3/library/pickle.html?highlight=__getstate__#object.__setstate__

bjlittle · 2019-12-06T07:43:55Z

lib/iris/common/metadata.py

+        namespace["_names"] = cls._names
+
+    # Dynamically create the class.
+    Metadata = type(name, bases, namespace)


Reference:

https://docs.python.org/3/library/functions.html?highlight=type#type

bjlittle · 2019-12-06T08:00:08Z

lib/iris/common/metadata.py

+        Dynamically created classes at runtime cannot be pickled, due to not
+        being defined at the top level of a module. As a result, we require to
+        use the __reduce__ interface to allow 'pickle' to recreate this class
+        instance, and dump and load instance state successfully.


Reference:

https://docs.python.org/3/library/pickle.html#what-can-be-pickled-and-unpickled

lbdreyer

What a big PR! But definitely an improvement to what we had before!

Overall I'm very much in support of the changes. Most of my comments relate to a few (mostly) minor points:

Naming
When reviewing this, I did find I was getting confused between what was returned by the MetadataFactory (returned by cube._metadata) and the instances of, for example, CubeMetadata (returned by cube.metadata). I think to provide more clarity between these I would suggest we rename a few things. (see individual comments below)
Test names
There was a bit of inconsistency with the naming of tests when the name of class/function being tests starts with an underscore.
The number of asserts per test
A lot of the tests grouped multiple self.assert*. I would prefer these are split up such that each independent test (i.e. test_standard_name) tests a single scenario
I believe we are missing some test coverage for _BaseMeta.__lt__

lib/iris/common/metadata.py

lbdreyer · 2019-12-11T15:37:52Z

lib/iris/common/metadata.py

+    @property
+    def _names(self):
+        """
+        A tuple containing the value of each name participating in the identify


Is this a typo? I would expect it to say identity?

Yup, good spot 👀

lbdreyer · 2019-12-16T15:39:08Z

lib/iris/tests/unit/common/mixin/test_get_valid_standard_name.py

@@ -4,15 +4,15 @@
 # See COPYING and COPYING.LESSER in the root of the repository for full
 # licensing details.
 """
-Unit tests for the :func:`iris._cube_coord_common.get_valid_standard_name`.
+Unit tests for the :func:`iris.common.mixin.get_valid_standard_name`.


Suggested change

Unit tests for the :func:`iris.common.mixin.get_valid_standard_name`.

Unit tests for the :func:`iris.common.mixin._get_valid_standard_name`.

And shouldn't this file be called .../common/mixin/test__get_valid_standard_name.py

@lbdreyer Yup. Originally it wasn't a private function. I ported it across into iris.common.mixin wrote the tests, then later changed it to be a private function. Forgot to also change the associated comment and test file name. Another good spot!

lbdreyer · 2019-12-16T17:06:13Z

lib/iris/tests/unit/common/mixin/test_CFVariableMixin.py

+        self.item = CFVariableMixin()
+        self.item._metadata = metadata
+
+    def test(self):


Can you split this into two tests; one for renaming to air_temperature and one for renaming to nope nope

lbdreyer · 2019-12-16T17:11:36Z

lib/iris/tests/unit/common/mixin/test_CFVariableMixin.py

+        self.item = CFVariableMixin()
+        self.item._metadata = metadata
+
+    def test_standard_name(self):


Could you split this up into three tests?
For readability it is nicer this way, but you are testing different scenarios here.

Could you also do this for the other tests in this test class?

lbdreyer · 2019-12-18T14:07:51Z

lib/iris/common/metadata.py

+_TOKEN_PARSE = re.compile(r"""^[a-zA-Z0-9][\w\.\+\-@]*$""")
+
+
+class _BaseMeta(ABCMeta):


I find this name a bit confusing as we have both METAdata and METAclass so it's unclear what the Meta refers to.
But I don't have a suggestion just yet. I will keep thinking....

@lbdreyer Yup, on reflection I do agree. It is confusing 😕

After a bit of thought (and mutual agreement) we're going with _NamedTupleMeta instead.

lbdreyer · 2019-12-18T14:09:08Z

lib/iris/tests/unit/common/metadata/test_BaseMetadata.py

+from iris.common.metadata import BaseMetadata
+
+
+class Test(tests.IrisTest):


It would be helpful to include tests for __lt__ on BaseMetadata

lbdreyer · 2019-12-18T14:47:26Z

lib/iris/common/mixin.py

@@ -5,43 +5,20 @@
 # licensing details.



I'm not so sure about the name of this module. mixindoesn't tell you much about what this module is supposed to contain. Unless you intend all mixin classes to live here?

@lbdreyer My intent here was simple, as in, it's a namespace for common mixin classes to live.

It might just be the case that CFVariableMixin will be our only mixin class, but at least we have scope for collecting such classes together in one place, rather than having them sprinkled around the codebase. HTH 😉

lbdreyer · 2019-12-18T15:00:01Z

lib/iris/common/metadata.py

+            raise ValueError(emsg.format(cls.__name__, bad))
+
+    # Define the name, (inheritance) bases and namespace of the dynamic class.
+    name = "Metadata"


Could we call this MetadataManager instead?

And perhaps call this function MetadataManagerFactory instead of MetadataFactory?

@lbdreyer Agreed. Let's do both for clarity.

lbdreyer · 2019-12-18T15:02:13Z

lib/iris/cube.py

@@ -775,6 +747,9 @@ def __init__(
        if isinstance(data, str):
            raise TypeError("Invalid data type: {!r}.".format(data))

+        # Configure the metadata manager.
+        self._metadata = MetadataFactory(CubeMetadata)


Could we call these self._metadata_manager?

Currently we have:
self.metadata - returns an instance of BaseMetadata which is a souped up namedtuple
self._metadata - returns the instance that MetadataFactory returns.
I find that a bit confusing.

Another good thing about self._metadata_manager is follows a similar naming pattern to self._data_manager

lbdreyer

Hi @bjlittle !

I have now fully completed reviewing this PR so it's ready for you to get started on making changes

lbdreyer · 2019-12-19T00:14:27Z

lib/iris/coords.py

+            self._metadata.climatological = False
+        if not self.units.is_time_reference():
+            self._metadata.climatological = False
+        return self._metadata.climatological


I have come around to the idea of this.
We do a is_time_reference check in the setter, so we are being careful with what is being set as having climatological bounds.
There is a risk is if someone modifies an already existing climatological coordinate to something like a latitude coordinate, without updating the climatological property. In these cases, I would prefer we ensure we save things out correctly (i.e. pick up that this coordinate shouldn't be climatological). It would only be a problem if they were to access coord._metadata.climatological directly, but they shouldn't be doing that
.
So this just needs some testing

@lbdreyer Thanks for taking the time to think about this one, much appreciated.

It was a bit cheeky of me to include this in this PR, but since I was in the space, I thought I'd try to address something that reeked of a potential issue for users.

I'll add some tests 😄

lib/iris/tests/unit/common/metadata/test_BaseMetadata.py

Co-Authored-By: lbdreyer <lbdreyer@users.noreply.github.com>

lbdreyer

👍 Happy with the changes. Now just to wait for travis then this can be merged!

bjlittle · 2019-12-19T15:46:39Z

@lbdreyer and @alastair-gemmell Awesome, thanks! 🍻 🎉

* common metadata api * rationalise _cube_coord_common into common * move state into metadata * MetadataFactory test coverage * temporarily pin back iris-grib * test coverage for iris.common.metadata._BaseMeta * test coverage for iris.common.mixin.LimitedAttributeDict * remove temporary iris-grib pin * review actions * Update lib/iris/tests/unit/common/metadata/test_BaseMetadata.py Co-Authored-By: lbdreyer <lbdreyer@users.noreply.github.com>

* PI-3478: Common metadata API (#3583) * common metadata api * rationalise _cube_coord_common into common * move state into metadata * MetadataFactory test coverage * temporarily pin back iris-grib * test coverage for iris.common.metadata._BaseMeta * test coverage for iris.common.mixin.LimitedAttributeDict * remove temporary iris-grib pin * review actions * Update lib/iris/tests/unit/common/metadata/test_BaseMetadata.py Co-Authored-By: lbdreyer <lbdreyer@users.noreply.github.com> * [FB] [PI-3478] Lenient metadata (#3739) * add lenient infra-structure * add metadata lenient __eq__ support * complete __eq__, combine and difference support * explicit inherited lenient_service + support equal convenience * fix attributes difference + lenient kwargs * make lenient public + minor tidy * rename MetadataManagerFactory to metadata_manager_factory * extend lenient_client decorator to support services registration * add lenient test coverage * purge qualname usage in metadata.py * support global enable for lenient services * support partial mapping metadata assignment * purge Lenient.__setattr__ from api * add BaseMetadata compare test coverage * metadata rationalisation * add BaseMetadata difference test coverage * added context manager ephemeral comment clarification * add BaseMetadata __ne__ test coverage * standardise lenient decorator closure names * add BaseMetadata equal test coverage * half dunder context * add AncillaryVariableMetadata test coverage * add additional AncillaryVariableMetadata test coverage * add CellMeasureMetadata test coverage * Clarify lenient_service operation + simplify code. * add CoordMetadata test coverage * add CubeMetadata test coverage * metadata tests use self.cls * fix typo * fix context manager ephemeral services * add logging * Pin pillow to make graphics tests work again. (#3630) * Fixed tests since Numpy 1.18 deprecation of non-int num arguments for linspace. (#3655) * Switched use of datetime.weekday() to datetime.dayofwk. (#3687) * New image hashes for mpl 3x2 (#3682) * New image hash for iris.test.test_plot.TestSymbols.test_cloud_cover with matplotlib 3.2.0. * Further images changes for mpl3x2. * Yet more updated image results. * fix sentinel uniqueness test failure * remove redundant cdm mapping test * difference returns None for no difference * protect Lenient and LENIENT private * privitise lenient framework and add API veneer * add explicit maths feature default * review actions * review actions * trexfeathers review actions * stephenworsley review actions Co-authored-by: Patrick Peglar <patrick.peglar@metoffice.gov.uk> Co-authored-by: Martin Yeo <40734014+trexfeathers@users.noreply.github.com> * [FB] [PI-3478] Lenient cube arithmetic (#3774) * initial cube arithmetic * support in-place cube resolve * fix non in-place broadcasting * remove temporary resolve scenario test * lenient/strict support for attributes dicts with numpy arrays * lenient/strict treatment of scalar coordinates * strict points/bounds matching * lenient/strict prepare local dim/aux/scalar coordinates * support extended broadcasting * always raise exception on points/bounds mismatch * ignore scalar points/bounds mismatches, lenient only * remove todos * tidy logger debugs * qualify src/tgt cube references in debug * Numpy rounding fix (#3758) ensure rounding is numpy like (maintains type) * avoid unittest.mock.sentinel copy issue * fast load np.int32 * fix cube maths doctest * fix iris.common.resolve logging configuration * fix prepare points/bounds + extra metadata cml * support mapping reversal based on free dims * var_name fix for lenient equality * add support for DimCoordMetadata * fix circular flag + support CoordMetadata and DimCoordMetadata exchange * fix circular issue for concatenate DimCoord->AuxCoord demotion * fix concatenate._CubeSignature sorted * minor tweaks * keep lenient_client private in maths * tidy maths * tidy iris.analysis.maths.IFunc * refactor IFunc test * polish in-place support * tidy metadata_resolve Co-authored-by: stephenworsley <49274989+stephenworsley@users.noreply.github.com> * rebase master fix-up for cube arithmetic * add missing new dependency to readthedocs.yml requirements Co-authored-by: lbdreyer <lbdreyer@users.noreply.github.com> Co-authored-by: Patrick Peglar <patrick.peglar@metoffice.gov.uk> Co-authored-by: Martin Yeo <40734014+trexfeathers@users.noreply.github.com> Co-authored-by: stephenworsley <49274989+stephenworsley@users.noreply.github.com>

bjlittle added Type: Enhancement Experience: High Release: Major Feature: Arithmetic labels Dec 5, 2019

bjlittle added this to the v3.0.0 milestone Dec 5, 2019

bjlittle assigned lbdreyer and alastair-gemmell Dec 5, 2019

bjlittle force-pushed the common-metadata-api branch 2 times, most recently from 8e571b0 to 14a9556 Compare December 6, 2019 07:28

bjlittle commented Dec 6, 2019

View reviewed changes

abooton changed the title ~~Common metadata API~~ PI-3478: Common metadata API Dec 6, 2019

bjlittle commented Dec 6, 2019

View reviewed changes

lbdreyer mentioned this pull request Dec 6, 2019

[PI] Implement new cube arithmetic proposal #3478

Closed

53 tasks

bjlittle force-pushed the common-metadata-api branch from 7e75975 to 8068848 Compare December 6, 2019 14:11

bjlittle added 8 commits December 6, 2019 14:29

common metadata api

e602ad8

rationalise _cube_coord_common into common

a586604

move state into metadata

e2fef55

MetadataFactory test coverage

e0a8d2a

temporarily pin back iris-grib

154222c

test coverage for iris.common.metadata._BaseMeta

6133af5

test coverage for iris.common.mixin.LimitedAttributeDict

9a46372

remove temporary iris-grib pin

137d4a2

bjlittle force-pushed the common-metadata-api branch from 8068848 to 137d4a2 Compare December 6, 2019 14:32

bjlittle mentioned this pull request Dec 6, 2019

Wrong error message for duplicate coordinate ESMValGroup/ESMValCore#373

Closed

stephenworsley mentioned this pull request Dec 16, 2019

(WIP) Extend new_axis() to help concatenate cubes with ancils #3600

Closed

lbdreyer requested changes Dec 18, 2019

View reviewed changes

lbdreyer requested changes Dec 19, 2019

View reviewed changes

review actions

59dba91

bjlittle force-pushed the common-metadata-api branch from 5db15a5 to 59dba91 Compare December 19, 2019 13:56

lbdreyer reviewed Dec 19, 2019

View reviewed changes

lib/iris/tests/unit/common/metadata/test_BaseMetadata.py Outdated Show resolved Hide resolved

Update lib/iris/tests/unit/common/metadata/test_BaseMetadata.py

fae0c89

Co-Authored-By: lbdreyer <lbdreyer@users.noreply.github.com>

lbdreyer approved these changes Dec 19, 2019

View reviewed changes

lbdreyer merged commit 569f6b6 into SciTools:cube-arithmetic Dec 19, 2019

bjlittle changed the title ~~PI-3478: Common metadata API~~ [FB] PI-3478: Common metadata API Jun 4, 2020

bjlittle changed the title ~~[FB] PI-3478: Common metadata API~~ [FB] [PI-3478] Common metadata API Jun 5, 2020

pp-mo mentioned this pull request Jun 12, 2020

Should aux factories have a coord-system ? #3738

Closed

bjlittle mentioned this pull request Jun 15, 2020

[FB] [PI-3478] Lenient metadata #3739

Merged

26 tasks

pp-mo mentioned this pull request Jun 25, 2020

Fix for Iris3 common-metadata SciTools/iris-grib#215

Closed

bjlittle mentioned this pull request Aug 13, 2020

[FB] [PI-3478] Lenient cube arithmetic #3774

Merged

20 tasks

bjlittle mentioned this pull request Aug 14, 2020

[PI-3478] Merge cube arithmetic feature branch #3785

Merged

bjlittle deleted the common-metadata-api branch February 15, 2021 13:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FB] [PI-3478] Common metadata API #3583

[FB] [PI-3478] Common metadata API #3583

bjlittle commented Dec 5, 2019 •

edited

Loading

bjlittle Dec 6, 2019

bjlittle Dec 6, 2019

bjlittle Dec 6, 2019

bjlittle Dec 6, 2019

bjlittle Dec 6, 2019

bjlittle Dec 6, 2019

bjlittle Dec 6, 2019

bjlittle Dec 6, 2019

lbdreyer left a comment

lbdreyer Dec 11, 2019

bjlittle Dec 19, 2019

lbdreyer Dec 16, 2019

bjlittle Dec 19, 2019

lbdreyer Dec 16, 2019

lbdreyer Dec 16, 2019

lbdreyer Dec 16, 2019

lbdreyer Dec 18, 2019

bjlittle Dec 19, 2019

lbdreyer Dec 18, 2019

lbdreyer Dec 18, 2019

bjlittle Dec 19, 2019

lbdreyer Dec 18, 2019

bjlittle Dec 19, 2019

lbdreyer Dec 18, 2019

lbdreyer left a comment •

edited

Loading

lbdreyer Dec 19, 2019

bjlittle Dec 19, 2019

lbdreyer left a comment

bjlittle commented Dec 19, 2019

		return (standard_name, long_name, var_name, stash_name)


		def MetadataFactory(cls, **kwargs):

	Unit tests for the :func:`iris.common.mixin.get_valid_standard_name`.
	Unit tests for the :func:`iris.common.mixin._get_valid_standard_name`.

		_TOKEN_PARSE = re.compile(r"""^[a-zA-Z0-9][\w\.\+\-@]*$""")


		class _BaseMeta(ABCMeta):

		from iris.common.metadata import BaseMetadata


		class Test(tests.IrisTest):

[FB] [PI-3478] Common metadata API #3583

[FB] [PI-3478] Common metadata API #3583

Conversation

bjlittle commented Dec 5, 2019 • edited Loading

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

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

lbdreyer left a comment

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

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

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

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

lbdreyer left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbdreyer left a comment

Choose a reason for hiding this comment

bjlittle commented Dec 19, 2019

bjlittle commented Dec 5, 2019 •

edited

Loading

lbdreyer left a comment •

edited

Loading