Add iris-esmf-regrid as a dependency #1809
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1809 +/- ##
=======================================
Coverage 91.50% 91.50%
=======================================
Files 202 202
Lines 10919 10927 +8
=======================================
+ Hits 9991 9999 +8
Misses 928 928
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
schlunma
left a comment
schlunma
left a comment
There was a problem hiding this comment.
Thanks @sloosvel, looks good!
One comment on documentation: I think it would make sense to add to the doc that we now not only support schemes that can be used within cube.regrid() (like ESMFAreaWeighted), but also schemes that are functions f(src_cube, grid_cube) -> Cube.
esmvalcore/preprocessor/_regrid.py
Outdated
| if isinstance(scheme_args, list): | ||
| if 'src_cube' in scheme_args: | ||
| scheme['src_cube'] = cube | ||
| if 'grid_cube' in scheme_args: | ||
| scheme['grid_cube'] = target_grid | ||
| loaded_scheme = obj(**scheme) | ||
|
|
There was a problem hiding this comment.
I think it would make sense to add this block to l.569. By doing this you will end up with a loaded_scheme for both the if and else statement. You could also restore the lines 574-576 then.
Can scheme_args be something else than None or a list? If not, I think
if scheme_args is not None:might be a little bit clearer. Also, can you add some comments what you do here? That would really help to understand the code better. E.g., why are you checking the args of obj, why are you extending them with the cube and tgrid, etc.
There was a problem hiding this comment.
Yes I thought it would fit better in those lines as well. However, there two schemes in iris-esmf.
The current main, supports the call to esmf_regrid.schemes.ESMFAreaWeighted(mdtol=0), which returns a scheme to be called by cube.regrid.
But esmf_regrid.schemes.regrid_rectilinear_to_rectilinear does not return a scheme, it's the regridding routine on it's own. It expects cubes as inputs and returns a regridded cube as output. And at that point the target cube has not been generated yet.
That is why the arguments are being checked. Because one scheme does not require them (unless we want to allow mdtol to be set as well, which is not possible right now), whereas another one fails if the src_cube and grid_cube are not given as inputs.
Would it work for you if the routine checked first the if isinstance(target_grid, str) block and then the if isinstance(scheme, dict) one? At first sight it does not look like it would break anything and it would be more organised.
There was a problem hiding this comment.
Ah yes, you are totally right, I didn't notice that target_grid is defined only later! I think swapping the blocks if isinstance(target_grid, str) and if isinstance(scheme, dict) is a good idea!
About the mdtol: I think it currently is possible to set this with
scheme:
reference: esmf_regrid.schemes.ESMFAreaWeighted
mdtol: 0.5There was a problem hiding this comment.
Ah true, then essentially the only tricky arguments were src_cube and grid_cube, since those cannot be passed in the recipe. I have updated the code with these changes
| if _attempt_irregular_regridding(cube, scheme): | ||
| cube = esmpy_regrid(cube, target_grid, scheme) | ||
| else: | ||
| if isinstance(loaded_scheme, iris.cube.Cube): | ||
| return loaded_scheme | ||
|
|
||
| cube = cube.regrid(target_grid, loaded_scheme) |
There was a problem hiding this comment.
| if _attempt_irregular_regridding(cube, scheme): | |
| cube = esmpy_regrid(cube, target_grid, scheme) | |
| else: | |
| if isinstance(loaded_scheme, iris.cube.Cube): | |
| return loaded_scheme | |
| cube = cube.regrid(target_grid, loaded_scheme) | |
| if _attempt_irregular_regridding(cube, scheme): | |
| cube = esmpy_regrid(cube, target_grid, scheme) | |
| elif isinstance(loaded_scheme, iris.cube.Cube): | |
| cube = loaded_scheme | |
| else: | |
| cube = cube.regrid(target_grid, loaded_scheme) |
Maybe clearer? Could you also add a comment here in which cases loaded_scheme is a Cube?
There was a problem hiding this comment.
Done, I have also updated the documentation.
Co-authored-by: Manuel Schlund <32543114+schlunma@users.noreply.github.com>
|
@ESMValGroup/esmvaltool-coreteam anyone willing to merge? |
|
@sloosvel two seconds, I want to turn on GA tests just for a wee, then will merge |
This PR adds
iris-esmf-regridas a dependency, adapts the regridding module so that the available schemes in the package (ESMFAreaWeightedandregrid_rectilinear_to_rectilinear) are supported and adds tests.Description
Closes #1794
Link to documentation:
Before you get started
Checklist
It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.
To help with the number pull requests: