Skip to content

Connectivity Class (AVD-1595)#3968

Merged
bjlittle merged 30 commits intoSciTools:mesh-data-modelfrom
trexfeathers:AVD-1595_topologycoord
Feb 8, 2021
Merged

Connectivity Class (AVD-1595)#3968
bjlittle merged 30 commits intoSciTools:mesh-data-modelfrom
trexfeathers:AVD-1595_topologycoord

Conversation

@trexfeathers
Copy link
Contributor

🚀 Pull Request

Description

Adds the Connectivity and accompanying ConnectivityMetadata classes - the first of many steps to allow Iris' core data model to represent the CF-UGRID unstructured mesh format in addition to the existing CF structured grid format. Note this is targeting the new mesh-data-model feature branch.

'Connectivity' name chosen since the most faithful interpretation of the UGRID conventions is that a topology is made up of one or more self-contained connectivities.

Implemented as another member of iris.coords, since Connectivity benefits from inheriting the majority of _DimensionalMetadata's functionality. Connectivity deviates slightly from existing _DimensionalMetadata children:

  • Will never be attached to a traditional cube dimension, so cube_dims is just overridden with NotImplementedError.
  • All the additional attributes all inter-relate - changing the value of just one will often render the others meaningless.
    As such none of them have a .setter method, and either need a special method to modify or the user is expected to create a new Connectivity with their new attribute values.
  • The values of the indices array attribute are only meaningful subject to several requirements regarding shape and dtype, so indices is heavily validated whenever it is set.

Consult Iris pull request check list

@bjlittle bjlittle self-assigned this Jan 29, 2021
"""
A CF-UGRID topology connectivity, describing the topological relationship
between two lists of dimensional elements. One or more connectivities
make up a CF-UGRID topology - a constituent of a CF-UGRID mesh.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite sure I agree on the description here...

The Connectivity is a definition (in terms of indices) of the interconnection between geometrical elements of a mesh, see here.

Could we make this doc-string a wee bit more specific to the role and purpose of Connectivity?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really struggled to come up with a decent description, any help is appreciated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bjlittle please could you provide a suggestion for us to talk over? Thanks 🍺

Copy link
Member

@bjlittle bjlittle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trexfeathers This is good, but there are quite a few things to mull over and address. So let's discuss them together.

I'd also question whether we want to inherit all the behaviour of the _DimensionalMetadata class i.e., do we need a slimmer base class of _DimensionalMetadata and inherit from that instead.

I'd also question inheriting the _DimensionMetadata.__getitem__ behaviour, which copies the payload... perhaps we may want to consider avoiding such standard behaviour for all mesh like objects.

Also, we may want to consider moving all this functionality to iris.experimental.ugrid - but that's definately out of scope of this PR

@trexfeathers
Copy link
Contributor Author

Back to you @bjlittle 🙂 Thanks for all your helpful comments, I'm pleased with how things have come out.

I've addressed everything except #3968 (comment), since we were planning to talk about this. I also haven't taken any actions on your general notes but there seems to be a general awareness of those anyway.

@bjlittle
Copy link
Member

bjlittle commented Feb 8, 2021

@trexfeathers Awesome work, thanks for sticking with it 👍 💯 🎉

Let's see the utility of Connectivity.indices_by_src vs Connectivity.__iter__ when we come to use it in anger... let's not second guess that right now. It will answer itself eventually (which is very soon 😉)

🍻

@bjlittle bjlittle merged commit 13ff24f into SciTools:mesh-data-model Feb 8, 2021
@trexfeathers
Copy link
Contributor Author

Thanks @bjlittle, much appreciated!

Let's see the utility of Connectivity.indices_by_src vs Connectivity.__iter__ when we come to use it in anger... let's not second guess that right now.

I suppose that was my point - before we know the answer I'd rather make it explicit rather than implicit.

@trexfeathers trexfeathers deleted the AVD-1595_topologycoord branch February 23, 2021 11:19
@trexfeathers trexfeathers mentioned this pull request Aug 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants