MeshCoordMetadata [AVD-1629] by pp-mo · Pull Request #4014 · SciTools/iris

pp-mo · 2021-02-17T11:48:31Z

🚀 Pull Request

Description

Adds MeshCoordMetadata class (with tests)
Preliminary to creating a MeshCoord

Note: also fixed some problems I stumbled over in existing metadata testing.
Some of which had already been copied into other tests! ...
There is a lot of boilerplate here :
much of the code in lib/iris/tests/unit/experimental/ugrid/test_MeshCoordMetadata.py is a total copy
of that in lib/iris/tests/unit/experimental/ugrid/test_ConnectivityMetadata.py (i.e. ultimately from #3968)

As we are in a hurry, we are resolved to fix that "later"...

Consult Iris pull request check list

stephenworsley

Looks good.

trexfeathers · 2021-02-17T13:59:13Z

Note: also fixed some problems I stumbled over in existing metadata testing.

Thanks for the catch @pp-mo!

Missed additional commits

pp-mo · 2021-02-17T14:40:39Z

Sorry all -- I failed to include all the changes, at first.
Now fixed

stephenworsley · 2021-02-18T14:59:27Z

lib/iris/experimental/ugrid.py

+# of lenient metadata services.
+# TODO: when included in 'iris.common.metadata', install each one directly ?
+for cls in (ConnectivityMetadata, MeshCoordMetadata):
+    op_names_and_collections = {


It might make more sense to define op_names_and_collections outside the loop.

@pp-mo If you do make convenience variables at the module level, could you also please purge them (del) from the namespace once you've finished using them please 👍

Awesome spot !
👍

Just as an afterthought : I'm a bit weak on this...
Would you consider it equivalent to define an __all__, or use only underscore-prefixed things in the package namespace, or is it really that much better to totally tidy it + if so why ?

( I always felt that trying to fix this was somewhat swimming against the tide, given the way that imports mess it all up anyway )

Done, I think.

bjlittle · 2021-02-19T08:47:35Z

lib/iris/experimental/ugrid.py

+            A list of different metadata member values.
+
+        """
+        # Perform "strict" difference for "cf_role", "start_index", "src_dim".


@pp-mo You just need to update this comment to align it with its members 👍

bjlittle

@pp-mo Awesome stuff! Just a few suggested changes, but LGTM. Great work 😀

We can start using the MeshCoordMetadata in anger to see how it feels, and revisit axis at a later date (soonish) to see what we think about it... plus we may have some actual concrete use case data that helps us weigh up and make an informed decision.

Regardlessly, changing the makeup of the MeshCoordMetadata is a cheap and easy step to take now 👍

🎉 🥳🍻

stephenworsley

Just a couple more comments about the testing along with the existing comments. Otherwise, this looks good!

stephenworsley · 2021-02-22T12:11:49Z

lib/iris/tests/unit/experimental/ugrid/test_MeshCoordMetadata.py

+    def test_op_strict_different(self):
+        lmetadata = self.cls(**self.values)
+        right = self.values.copy()
+        right["long_name"] = self.dummy


I feel like it would be more appropriate for this line to be equivalent to this line in test_op_lenient_different

iris/lib/iris/tests/unit/experimental/ugrid/test_MeshCoordMetadata.py

Line 154 in 0d208dd

right["units"] = self.dummy

This would reinforce the fact that the reason for the difference in behaviour is due to changing the lenient behaviour and not the choice of value changed.

Effectively I "inherited" all this from the test_ConnectivityMetadata (except it is really copied of course).
I have now "rationalised" the use of the non-member metadata in these tests : uses of "units" and "var_name" have been replaced with "long_name". I thought that made some sense, as long_name can be just anything whereas units=None would read back as "Unit('unknown')" if using a real object.

stephenworsley · 2021-02-22T13:49:26Z

lib/iris/tests/unit/experimental/ugrid/test_MeshCoordMetadata.py

+    def test_op_strict_different(self):
+        lmetadata = self.cls(**self.values)
+        right = self.values.copy()
+        right["long_name"] = self.dummy


Same as above comment.

stephenworsley · 2021-02-22T13:55:50Z

lib/iris/tests/unit/experimental/ugrid/test_MeshCoordMetadata.py

+        left = self.values.copy()
+        lmetadata = self.cls(**left)
+        right = self.values.copy()
+        right["long_name"] = self.dummy


Same as above comment.

pp-mo · 2021-02-22T15:29:38Z

Hoping that latest addresses @bjlittle comments. Ping!

Still thinking about @stephenworsley tests changes.
Will respond on that shortly..

pp-mo · 2021-02-22T15:59:24Z

Rebased to fix problem with tests.

bjlittle · 2021-02-22T23:11:23Z

lib/iris/experimental/ugrid.py

-#: Convenience collection of lenient metadata difference services.
-SERVICES_DIFFERENCE.append(ConnectivityMetadata.difference)
-SERVICES.append(ConnectivityMetadata.difference)
+    _members = ("mesh", "location", "axis")


@pp-mo Whoa... are you really suggesting that mesh is metadata here? As in an instance of the forthcoming iris.experimental.ugrid.Mesh ?

This is the payload of the MeshCoord, right? If so, then I really don't agree with this... it's akin to making points and bounds members of the CoordMetadata or DimCoordMetadata, or making the data a member of the CubeMetadata 👎

Or do you intend mesh to be something completely different? 🤔

Obviously having the actual Mesh as part of the metadata wouldn't work, but given the idea of shared Meshes I'd say some sort of mesh identifier is an important component of the MeshCoord metadata.

If mesh is a cheap comparison, then yes this could be part of the metadata, but until we know better I think we should err on the side of this not being part the metadata

Also, __eq__ hasn't been defined within iris.experimental.ugrid.Mesh for this very reason... we're kinda sitting on the fence until we know better

Ok removed it

pp-mo · 2021-02-23T12:45:28Z

Ok, now removed 'mesh' from the metadata, and rebased to accomodate the latest on the mesh-data-model target.
Also tidied up the 'service collections' adjustments, now all handled at the end of 'ugrid.py'

pp-mo · 2021-02-23T12:47:13Z

@stephenworsley @bjlittle think we have addressed all previous issues now.
Sorry about the rebase.
Please re-review !

bjlittle · 2021-02-23T13:02:56Z

@pp-mo and @stephenworsley LGTM 👍

pp-mo requested review from bjlittle and trexfeathers February 17, 2021 11:48

pp-mo changed the title ~~Fix some problems with existing tests.~~ MeshCoordMetadata Feb 17, 2021

pp-mo requested a review from stephenworsley February 17, 2021 12:19

pp-mo changed the title ~~MeshCoordMetadata~~ MeshCoordMetadata [AVD-1629] Feb 17, 2021

stephenworsley previously approved these changes Feb 17, 2021

View reviewed changes

stephenworsley reviewed Feb 18, 2021

View reviewed changes

bjlittle reviewed Feb 19, 2021

View reviewed changes

bjlittle requested changes Feb 19, 2021

View reviewed changes

bjlittle assigned stephenworsley Feb 22, 2021

stephenworsley requested changes Feb 22, 2021

View reviewed changes

pp-mo force-pushed the meshcoordmeta branch from 4e4d05e to c97659c Compare February 22, 2021 15:56

stephenworsley approved these changes Feb 22, 2021

View reviewed changes

bjlittle reviewed Feb 22, 2021

View reviewed changes

pp-mo added 8 commits February 23, 2021 12:14

Fix some problems with existing tests.

8afaa4d

Add MeshCoordMetadata, and tests.

fb8b2f4

Add tests.

ee4926f

Tidy import code + cleaner package namespace.

5f4bc30

Fix duplicated comment.

c07fa3f

Rationalise which non-member metadata item in testing.

d702d65

Remove 'mesh' from MeshCoordMetadata.

30808dd

Common code to add metadata methods into lenient service collections.

39cf78e

pp-mo force-pushed the meshcoordmeta branch from 42b5bf1 to 39cf78e Compare February 23, 2021 12:43

bjlittle approved these changes Feb 23, 2021

View reviewed changes

stephenworsley merged commit e719ae0 into SciTools:mesh-data-model Feb 23, 2021

pp-mo deleted the meshcoordmeta branch February 23, 2021 14:06

trexfeathers mentioned this pull request Aug 2, 2021

Ugrid whatsnew #4236

Merged

Conversation

pp-mo commented Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🚀 Pull Request

Description

Uh oh!

stephenworsley left a comment

Choose a reason for hiding this comment

Uh oh!

trexfeathers commented Feb 17, 2021

Uh oh!

pp-mo commented Feb 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjlittle Feb 19, 2021 • 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 Feb 19, 2021 • 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 Feb 22, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjlittle left a comment

Choose a reason for hiding this comment

Uh oh!

stephenworsley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pp-mo Feb 22, 2021 • 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!

pp-mo commented Feb 22, 2021

Uh oh!

pp-mo commented Feb 22, 2021

Uh oh!

bjlittle Feb 22, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pp-mo commented Feb 23, 2021

Uh oh!

pp-mo commented Feb 23, 2021

Uh oh!

bjlittle commented Feb 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

pp-mo commented Feb 17, 2021 •

edited

Loading

bjlittle Feb 19, 2021 •

edited

Loading

pp-mo Feb 19, 2021 •

edited

Loading

pp-mo Feb 22, 2021 •

edited

Loading

pp-mo Feb 22, 2021 •

edited

Loading

bjlittle Feb 22, 2021 •

edited

Loading