-
Notifications
You must be signed in to change notification settings - Fork 283
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
(Regridder unification) improve curvilinear regridding, generalise _create_cube #4807
Conversation
@stephenworsley Fancy rebasing from This will enable CI for GHA 👍 |
0266a77
to
e445760
Compare
The goal with this PR is to demonstrate the possibility of a common structure that regridders can follow. That structure would look something like the pseudocode bellow. The Rough proposed common structure of regridders:def regrid(data, dims, regrid_info):
...
return new_data
def create_cube(data, src, src_dims, tgt_coords, callback):
...
return result_cube
def _prepare(src, tgt):
...
return regrid_info
def _perform(cube, regrid_info):
...
dims = get_dims(cube)
data = regrid(cube.data, dims, regrid_info)
callback = functools.partial(regrid, regrid_info=regrid_info)
return create_cube(data, src, dims, tgt_coords, callback)
class Regridder:
def __init__(src, tgt):
...
self._regrid_info = _prepare(src, tgt)
def __call__(src):
...
return _perform(src, self._regrid_info) |
e445760
to
6252e9b
Compare
I just added a benchmark for curvilinear regridding (whose code I changed the most substantially), when I run it locally it takes about 0.2 seconds, with the old code it takes about 5 seconds. |
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.
This is a great improvement and a fantastic speed up!
I still have a few things I want to check, but I wanted to submit the comments I had so far in case you wanted to start working on them
lib/iris/tests/unit/analysis/regrid/test__CurvilinearRegridder.py
Outdated
Show resolved
Hide resolved
613153c
to
a6c38f3
Compare
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
a6c38f3
to
f68115d
Compare
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.
Looking good to me! Thanks @stephenworsley
An attempt at demonstrating some of the ideas in #4754. That is, using a common function to create cubes from regridded data.
This addresses two of the bullet points in this issue:
CurvilinearRegridder
to work on whole cubes rather than slices._create_cube
so that they can be consistent with the regridder they are being used in.This PR replaces the cube creation code in the
RectilinearRegridder
,AreaWeightedRegridder
and theCurvilinearRegridder
so that they both call the same function,_create_cube
. To accomplish this, surrounding code has been altered to be compatible with this new function. Whiole the behaviour of these regridders should otherwise remain the same, this has lead to some improvements in the functionality:CurvilinearRegridder
ought to be more efficient now that they are running on full data rather than slices (TODO: check if this is a measurable performance improvement.)CurvilinearRegridder
is now able to handle derived coordinates in the same way that theAreaWeightedRegridder
andRectilinearRegridder
do (TODO: add tests to verify that this works).AreaWeightedRegridder
, the associated functionregrid_area_weighted_rectilinear_src_and_grid
is now able to handle derived coordinates when one of the grid coordinates is scalar.RectilinearRegridder
, the underlying functions should now be able to handle scalar grid coords. This should make it easier to add this in a subsequent PR.