-
Notifications
You must be signed in to change notification settings - Fork 280
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
Improved use of UGRID terminology #4498
Conversation
There was a problem hiding this 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.
There was a problem hiding this 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
After discussion with @stephenworsley have proposed b7ec240 . The property is now named
|
There was a problem hiding this 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.
There was a problem hiding this 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
There was a problem hiding this 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.
The new fixes look good, I'm happy to merge this now. |
🚀 Pull Request
Description
Closes #4492. Altered variable names throughout Iris' UGRID support to make two changes:
element
andlocation
for more precision.element
=node
/edge
/face
/volume
location
= whichelement
is the subject of thisCube
/MeshCoord
/Connectivity
?src
/tgt
terminologies inConnectivity
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