Unstructured Scheme - Cube Creation 3D by stephenworsley · Pull Request #47 · SciTools/iris-esmf-regrid

stephenworsley · 2021-03-30T11:51:59Z

Expands on #39 by adding the ability to handle cubes with additional dimensions. Specifically, this copies from the source cube all DimCoords and AuxCoords which do not span the mesh dimension. Derived coordinates are a more complicated case which may be added in a future PR.

codecov · 2021-03-30T11:54:30Z

Codecov Report

Merging #47 (1bb659f) into unstructured_scheme (7c01ece) will increase coverage by 0.11%.
The diff coverage is 100.00%.

@@                   Coverage Diff                   @@
##           unstructured_scheme      #47      +/-   ##
=======================================================
+ Coverage                98.15%   98.27%   +0.11%     
=======================================================
  Files                       16       16              
  Lines                      652      695      +43     
=======================================================
+ Hits                       640      683      +43     
  Misses                      12       12

Impacted Files	Coverage Δ
esmf_regrid/experimental/unstructured_scheme.py	`97.18% <100.00%> (+0.35%)`	⬆️
...erimental/unstructured_scheme/test__create_cube.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c01ece...1bb659f. Read the comment docs.

jamesp

Looking good @stephenworsley , a few comments inline

jamesp · 2021-04-08T08:30:52Z

esmf_regrid/experimental/unstructured_scheme.py

+            dims = src_cube.coord_dims(coord)
+            if hasattr(coord, "mesh") or mesh_dim in dims:
+                continue
+            dims = [dim if dim < mesh_dim else dim + 1 for dim in dims]


Can we have a comment to explain this line?
I think from the continue statement above, dim will never equal mesh_dim, but explaining why it's going to be necessary to shift all later dimensions by 1 would make it clearer.

If this code has been copied from iris then this comment probably belongs there too!

This line is new.

jamesp · 2021-04-08T08:35:59Z

esmf_regrid/experimental/unstructured_scheme.py

+                continue
+            dims = [dim if dim < mesh_dim else dim + 1 for dim in dims]
+            result_coord = coord.copy()
+            add_method(result_coord, dims)


I think I get this, but slightly confusing as we're not returning anything from this function. Is this because add_method is always going to be a method on a cube to which we are now appending these new coordinates, so the state change happens on the add_method owner?

jamesp · 2021-04-08T08:40:25Z

esmf_regrid/experimental/unstructured_scheme.py

@@ -88,10 +88,6 @@ def _create_cube(data, src_cube, mesh_dim, grid_x, grid_y):



See Line 85: describes a mesh object, but this is no longer part of the _create_cube signature

jamesp · 2021-04-08T08:41:59Z

esmf_regrid/tests/unit/experimental/unstructured_scheme/test__create_cube.py

+    grid_y = DimCoord(np.arange(2), standard_name="latitude")
+
+    cube = _create_cube(data, src_cube, mesh_dim, grid_x, grid_y)
+    src_metadata = src_cube.metadata


should _create_cube be handling metadata as well?

_create_cube already handles metadata, this is also the case in iris.

Ah yes, sorry I misread this section. You're getting the source metadata to attach to the expected_cube, not to cube.

jamesp · 2021-04-08T08:43:19Z

esmf_regrid/tests/unit/experimental/unstructured_scheme/test__create_cube.py

+    src_cube.attributes = {"a": 1}
+    src_cube.standard_name = "air_temperature"
+    scalar_height = AuxCoord([5], units="m", standard_name="height")
+    scalar_time = DimCoord([10], units="s", standard_name="time")


Is this right? Adding a DimCoord as an add_aux_coord?

This is a bit of Iris weirdness. While DimCoord and AuxCoord are different classes, a cube also has two "boxes" to put coords in: _aux_coords_and_dims and _dim_coords_and_dims. Somewhat counterintuitively, _aux_coords_and_dims can contain DimCoords. In fact, since this is a scalar coordinate, it has to be added to _aux_coords_and_dims since it does not describe a dimension (all coords in _dim_coords_and_dims must be associated with a dimension). Scalar DimCoords will be generated when a cube is sliced, so these are worth considering and testing with.

Great, thanks for the explanation. With that in mind, are we comfortable these tests are sufficiently complete in their coverage for now?

I think so, there's still some stuff like derived coordinates which we might want to add in the future, but I think this covers the main stuff.

* handle extra dims in _create_cube * add test * lint fix * address review comment

* unstructured_scheme: Add grid to mesh scheme (SciTools#96) Unstructured scheme integration test (SciTools#66) Updated lockfiles according to branch requirement spec. Update Version to v0.1.dev2 (SciTools#59) Update version to v0.1.dev1 (SciTools#58) Unstructured Scheme - Extra Dims (SciTools#55) add __init__ to tests (SciTools#56) Unstructured Scheme - Additional Polish (docstrings and test coverage) (SciTools#53) Unstructured Scheme - Cube Creation 3D (SciTools#47) fix test (SciTools#52) Update unstructured_scheme Feature Branch (SciTools#51) # Conflicts: # esmf_regrid/__init__.py # requirements/nox.lock/py36-linux-64.lock # requirements/nox.lock/py37-linux-64.lock # requirements/nox.lock/py38-linux-64.lock

* unstructured_scheme: (22 commits) Check mesh equality on MeshToGridESMFRegridder call (SciTools#138) Formalise regridder file format (SciTools#137) Fix issue 135 (mesh to grid chunking problems) (SciTools#136) Add load/save benchmarks (SciTools#132) Regridder load/saving (SciTools#130) Update dependencies (SciTools#128) Perform scalability for larger grids (SciTools#122) Add performance tests (SciTools#117) change iris source (SciTools#115) Unstructured scheme lazy regridding (with performance tests) (SciTools#111) Updating feature branch unstructured_scheme from 44d6048 to head of main, e528cbf Add grid to mesh scheme (SciTools#96) Unstructured scheme integration test (SciTools#66) Updated lockfiles according to branch requirement spec. Update Version to v0.1.dev2 (SciTools#59) Update version to v0.1.dev1 (SciTools#58) Unstructured Scheme - Extra Dims (SciTools#55) add __init__ to tests (SciTools#56) Unstructured Scheme - Additional Polish (docstrings and test coverage) (SciTools#53) Unstructured Scheme - Cube Creation 3D (SciTools#47) ... # Conflicts: # benchmarks/benchmarks/ci/esmf_regridder.py # benchmarks/benchmarks/generate_data.py # requirements/nox.lock/py36-linux-64.lock # requirements/nox.lock/py37-linux-64.lock # requirements/nox.lock/py38-linux-64.lock # requirements/py36.yml

jamesp reviewed Apr 8, 2021

View reviewed changes

stephenworsley added 4 commits April 8, 2021 17:12

handle extra dims in _create_cube

fcee4ee

add test

ea8007a

lint fix

5783808

address review comment

1bb659f

stephenworsley force-pushed the unstructured_scheme_create_cube_3d branch from 420904e to 1bb659f Compare April 8, 2021 16:13

stephenworsley changed the base branch from unstructured_scheme_original to unstructured_scheme April 8, 2021 16:15

stephenworsley closed this Apr 9, 2021

stephenworsley reopened this Apr 9, 2021

jamesp merged commit d4d7a07 into SciTools:unstructured_scheme Apr 9, 2021

stephenworsley added a commit to stephenworsley/iris-esmf-regrid that referenced this pull request Jun 2, 2021

Unstructured Scheme - Cube Creation 3D (SciTools#47)

f9bdbba

* handle extra dims in _create_cube * add test * lint fix * address review comment

trexfeathers mentioned this pull request Feb 21, 2022

update changelog #151

Merged

		@@ -88,10 +88,6 @@ def _create_cube(data, src_cube, mesh_dim, grid_x, grid_y):

Conversation

stephenworsley commented Mar 30, 2021

Uh oh!

codecov bot commented Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jamesp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Mar 30, 2021 •

edited

Loading