-
Notifications
You must be signed in to change notification settings - Fork 300
Improved use of UGRID terminology #4498
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
Conversation
stephenworsley
left a comment
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.
pp-mo
left a comment
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
|
stephenworsley
left a comment
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.
stephenworsley
left a comment
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.
stephenworsley
left a comment
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.
pp-mo
left a comment
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:
elementandlocationfor more precision.element=node/edge/face/volumelocation= whichelementis the subject of thisCube/MeshCoord/Connectivity?src/tgtterminologies inConnectivityto reduce name clashes and confusion.src_location->location_elementtgt_location->connected_elementsrc_dim->location_axistgt_dim->connected_axis(previously discussed removal but found it was offering convenience in several places)
Consult Iris pull request check list