Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved use of UGRID terminology #4498

Merged
merged 11 commits into from
Jan 26, 2022

Conversation

trexfeathers
Copy link
Contributor

@trexfeathers trexfeathers commented Jan 12, 2022

🚀 Pull Request

Description

Closes #4492. Altered variable names throughout Iris' UGRID support to make two changes:

  • Use both element and location for more precision.
    • element = node/edge/face/volume
    • location = which element is the subject of this Cube/MeshCoord/Connectivity?
    • Enables the second change below
  • Reworked the src/tgt terminologies in Connectivity to reduce name clashes and confusion.
    • src_location -> location_element
    • tgt_location -> connected_element
    • src_dim -> location_axis
    • tgt_dim -> connected_axis
      (previously discussed removal but found it was offering convenience in several places)

Consult Iris pull request check list

Copy link
Contributor

@stephenworsley stephenworsley left a comment

Choose a reason for hiding this comment

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

I think we're getting there, though I think there is still a bit of confusion in the documentation over location_element the type (which is what is stored in the attribute), 'location element' used to denote an individual element and a confusing use of location element to refer to the list of connected elements associated with that individual location element.

lib/iris/experimental/ugrid/mesh.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/mesh.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/mesh.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/mesh.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/mesh.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/mesh.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/mesh.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/mesh.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/mesh.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/mesh.py Outdated Show resolved Hide resolved
Copy link
Member

@pp-mo pp-mo left a comment

Choose a reason for hiding this comment

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

Some slightly random thoughts.

Raises some general points, which aren't necessarily best addressed here + now:

  • not sure about our use of the word 'topology'
  • still uncomfortable about the Mesh API + wish we weren't releasing this -- but it is experimental

lib/iris/experimental/ugrid/load.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/mesh.py Show resolved Hide resolved
lib/iris/experimental/ugrid/mesh.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/mesh.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/mesh.py Outdated Show resolved Hide resolved
@trexfeathers
Copy link
Contributor Author

After discussion with @stephenworsley have proposed b7ec240 . The property is now named location.

  • Consistent with the terminology in MeshCoords and Cubes.
  • Differentiates between:
    • location - the string naming the element type e.g. face.
    • location element - an instance of the location e.g. a single face
  • Allows documentation to be phrased like:
    image
    • Avoids misleadingly referring to the attribute as an instance of the location when it's actually just the type of the location.
    • Still provides a link if the reader needs to learn more.
    • You can actually substitute the value of location into the sentence. So "... each face element ..." in the example above.

@trexfeathers trexfeathers marked this pull request as draft January 17, 2022 14:34
@trexfeathers trexfeathers marked this pull request as ready for review January 17, 2022 15:57
Copy link
Contributor

@stephenworsley stephenworsley left a comment

Choose a reason for hiding this comment

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

Looks good, just a few minor comments about the documentation.

lib/iris/experimental/ugrid/mesh.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/mesh.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/mesh.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/mesh.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/mesh.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/mesh.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/mesh.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/mesh.py Outdated Show resolved Hide resolved
Copy link
Contributor

@stephenworsley stephenworsley left a comment

Choose a reason for hiding this comment

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

Just one more tiny thing and I think we're good.

lib/iris/experimental/ugrid/mesh.py Outdated Show resolved Hide resolved
Copy link
Contributor

@stephenworsley stephenworsley left a comment

Choose a reason for hiding this comment

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

Looks good.

Copy link
Member

@pp-mo pp-mo left a comment

Choose a reason for hiding this comment

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

Couple of minor readability points noted.
Otherwise reads really well now, I think.

lib/iris/experimental/ugrid/mesh.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/mesh.py Outdated Show resolved Hide resolved
lib/iris/experimental/ugrid/mesh.py Outdated Show resolved Hide resolved
@stephenworsley
Copy link
Contributor

The new fixes look good, I'm happy to merge this now.

@stephenworsley stephenworsley merged commit 633ed17 into SciTools:main Jan 26, 2022
@trexfeathers trexfeathers deleted the connectivity_terms branch March 31, 2022 13:45
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.

Rename Connectivity src and tgt
4 participants