-
Notifications
You must be signed in to change notification settings - Fork 17
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
Unstructured Scheme - Basic MeshInfo Handling #36
Unstructured Scheme - Basic MeshInfo Handling #36
Conversation
Codecov Report
@@ Coverage Diff @@
## unstructured_scheme #36 +/- ##
=======================================================
+ Coverage 91.08% 92.08% +1.00%
=======================================================
Files 11 12 +1
Lines 415 455 +40
=======================================================
+ Hits 378 419 +41
+ Misses 37 36 -1
Continue to review full report at Codecov.
|
@@ -30,6 +30,8 @@ env: | |||
PIP_CACHE_PACKAGES: "pip setuptools wheel nox pyyaml" | |||
# Conda packages to be installed. | |||
CONDA_CACHE_PACKAGES: "nox pip pyyaml" | |||
# Use specific custom iris source feature branch. | |||
IRIS_SOURCE: "github:mesh-data-model" |
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.
@stephenworsley Easy as 🥧
That's really pleasing to see 😄
def _cube_to_MeshInfo(cube): | ||
# Returns a MeshInfo object describing the mesh of the cube. | ||
pass | ||
def _mesh_to_MeshInfo(mesh): |
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.
The pros we had looked quite useful. Could we keep a bit of a doc string here?
def _mesh_to_MeshInfo(mesh): | ||
assert mesh.topology_dimension == 2 | ||
meshinfo = MeshInfo( | ||
np.stack([coord.points for coord in mesh.node_coords], axis=-1), |
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.
The order in which mesh.node_coords returns its respective coordinates is controled by https://github.com/SciTools/iris/blob/3b8f492e1a16c9cbd50dd3758f96f3178c40c737/lib/iris/experimental/ugrid.py#L2126
|
||
def _example_mesh(): | ||
"""Generate a global mesh with a square pyramid topology.""" | ||
fnc_array = [ |
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.
Maybe add a comment in here:
# construct face-node-connectivity (fnc)
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.
Agree, a bit of explanation of what is being made here would be helpful
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.
ASCII diagram possible?!
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.
great! excellent artistic skills 🎨
lons = AuxCoord(lon_values, standard_name="longitude") | ||
lats = AuxCoord(lat_values, standard_name="latitude") | ||
mesh = Mesh(2, ((lons, "x"), (lats, "y")), fnc) | ||
return mesh |
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.
Polygons are described in such a way that nodes are traversed clockwise. There is still some question about what orientations UGRID allows and what orientations ESMF can handle. If UGRID can allow more than ESMF can handle, we may have to catch and handle such cases ourselves.
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.
Having looked at the UGRID conventions again, the only case in which both clockwise and anticlockwise orientations are allowed is for Fully 3D unstructured meshes. In all other cases, anticlockwise orientation is specified.
ESMF documentation seems to imply that oriantation of face nodes (being visited clockwise/anticlockwise) is important and ought to be anticlockwise (https://earthsystemmodeling.org/esmpy_doc/release/ESMF_8_0_1/html/mesh.html#ESMF.api.mesh.Mesh.add_elements). However, current tests seem to imply that either orientation is equivalent for calculations. After some more investigation, it looks like orientation is inportant in the cases where the described polygon has concave parts. When this is the case, the clockwise case will cause ESMF to error. |
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.
Great work @stephenworsley! A couple of comments from our convo, otherwise good to go!
@@ -97,14 +102,13 @@ def _regrid_unstructured_to_rectilinear__prepare(src_mesh_cube, target_grid_cube | |||
# TODO: Record appropriate dimensions (i.e. which dimension the mesh belongs to) | |||
|
|||
grid_x, grid_y = get_xy_coords(target_grid_cube) | |||
mesh, mesh_dim = _get_mesh_and_dim(src_mesh_cube) |
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.
Is there a specific reason _get_mesh_and_dim
and _mesh_to_MeshInfo
are separate and distinct? I see we return mesh_dim
below, but could/should mesh_dim
be captured in the MeshInfo object?
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.
As discussed, either rename or just document that mesh_dim
is not the "dimension of the mesh".
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.
The way this regridder works, it's possible to initialise a regridder so that it will work with any cube that contains the same mesh (checking that the mesh is actally the same is planned to be added later). Because of this, mesh_dim
shouldn't be captured at the MeshInfo level since it is specific to the cube being regrid. You may note that this is thrown away in the regridding class during initialisation https://github.com/SciTools-incubator/iris-esmf-regrid/blob/98a339fa29afb1a19f044f1d0b665b1af463a4fd/esmf_regrid/experimental/unstructured_scheme.py#L169 and reconstructed during the call from the new cube https://github.com/SciTools-incubator/iris-esmf-regrid/blob/98a339fa29afb1a19f044f1d0b665b1af463a4fd/esmf_regrid/experimental/unstructured_scheme.py#L176-L178
This redundancy is due to the prepare and perform functions also being used in the following function
https://github.com/SciTools-incubator/iris-esmf-regrid/blob/98a339fa29afb1a19f044f1d0b665b1af463a4fd/esmf_regrid/experimental/unstructured_scheme.py#L140-L144
which does regridding all at once and does not cache. All this is behaviour inherited from iris.
As for why _get_mesh_and_dim
and _mesh_to_MeshInfo
should be distinct, during the call, _get_mesh_and_dim
serves a different roll (fetching the mesh_dim specific to the cube and fetching the mesh for future checking) and the MeshInfo object has already been created and recorded.
|
||
def _example_mesh(): | ||
"""Generate a global mesh with a square pyramid topology.""" | ||
fnc_array = [ |
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.
Agree, a bit of explanation of what is being made here would be helpful
|
||
def _example_mesh(): | ||
"""Generate a global mesh with a square pyramid topology.""" | ||
fnc_array = [ |
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.
ASCII diagram possible?!
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 great @stephenworsley, LGTM. As you develop this API, we should revisit whether the pyramid test is more widely applicable than testing MeshInfo.
|
||
def _example_mesh(): | ||
"""Generate a global mesh with a square pyramid topology.""" | ||
fnc_array = [ |
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.
great! excellent artistic skills 🎨
* Unstructured scheme skeleton (#31) * create skeleton of unstructured regridding code This code is primarily based on Iris's current Area weighted regridding scheme, with the required adaptions for accepting unstructred UGRID data. * Unstructured Scheme - Basic GridInfo Handling (#32) Add basic `GridInfo` function and associated tests. It returns an ESMF regridding object for gridded data. * Unstructured scheme iris source (#33) * refresh cirrus-ci and nox * add iris artifact support * deal with special-case cirrus-ci quoting * review actions * Unstructured Scheme - Basic MeshInfo Handling (#36) * provide iris Mesh to MeshInfo conversion * test against iris feature branch * lint fixes and documentation * fix typo * address review comments. * change tests to better reflect UGRID orientation * address review comments. * address review comments * ASCII art fix * Unstructured Scheme - Cube Creation 2D (#39) * support creation of 2D cubes * lint fixes * rewrite test * remove mesh from _create_cube arguments * add TODO * add TODO * Unstructured Scheme - Mesh Fetching (#46) * support mesh fetching * fix docstring * lint fix * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * add docstrings * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * add and restructure tests * fix test * fix test * address review comments * address review comments Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Bill Little <bill.little@metoffice.gov.uk> Co-authored-by: Anna Booton <anna.booton@metoffice.gov.uk> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
* Unstructured scheme skeleton (SciTools#31) * create skeleton of unstructured regridding code This code is primarily based on Iris's current Area weighted regridding scheme, with the required adaptions for accepting unstructred UGRID data. * Unstructured Scheme - Basic GridInfo Handling (SciTools#32) Add basic `GridInfo` function and associated tests. It returns an ESMF regridding object for gridded data. * Unstructured scheme iris source (SciTools#33) * refresh cirrus-ci and nox * add iris artifact support * deal with special-case cirrus-ci quoting * review actions * Unstructured Scheme - Basic MeshInfo Handling (SciTools#36) * provide iris Mesh to MeshInfo conversion * test against iris feature branch * lint fixes and documentation * fix typo * address review comments. * change tests to better reflect UGRID orientation * address review comments. * address review comments * ASCII art fix * Unstructured Scheme - Cube Creation 2D (SciTools#39) * support creation of 2D cubes * lint fixes * rewrite test * remove mesh from _create_cube arguments * add TODO * add TODO * Unstructured Scheme - Mesh Fetching (SciTools#46) * support mesh fetching * fix docstring * lint fix * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * add docstrings * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * add and restructure tests * fix test * fix test * address review comments * address review comments Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Bill Little <bill.little@metoffice.gov.uk> Co-authored-by: Anna Booton <anna.booton@metoffice.gov.uk> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Introduces similar functionality to #32 for the handling of iris Mesh objects. That is, the translation of Mesh objects to MeshInfo objects (extraction of Mesh objects from cubes will be done in a future PR). Also activates #33 so that testing is now done against the iris feature branch
mesh-data-model
which contains the Mesh object