Skip to content
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

Merged
merged 27 commits into from Nov 11, 2022

Conversation

stephenworsley
Copy link
Contributor

@stephenworsley stephenworsley commented Jun 16, 2022

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:

  • Rewrite the calculations in CurvilinearRegridder to work on whole cubes rather than slices.
  • Rewrite the way callbacks work in _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 the CurvilinearRegridder 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:

  • The calculations in the 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.)
  • The CurvilinearRegridder is now able to handle derived coordinates in the same way that the AreaWeightedRegridder and RectilinearRegridder do (TODO: add tests to verify that this works).
  • While this functionality is not yet in the AreaWeightedRegridder, the associated function regrid_area_weighted_rectilinear_src_and_grid is now able to handle derived coordinates when one of the grid coordinates is scalar.
  • While this functionality is not yet in the 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.

@bjlittle
Copy link
Member

@stephenworsley Fancy rebasing from main?

This will enable CI for GHA 👍

@stephenworsley
Copy link
Contributor Author

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 _prepare and regrid functions would have to be specific to each regridder, most of the other functions may have small differences due to the types of "grids" they deal with, but there is room for code reuse. In particular, this PR tries to demonstrate that create_cube can be made more generic so that one function can be reused by many regridders (with the bonus that these regridders will then be able to handle derived coordinates).

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)

@stephenworsley stephenworsley changed the title Regridder unification ideas (Regridder unification) improve curvilinear regridding, generalise _create_cube Sep 30, 2022
This was referenced Oct 5, 2022
@stephenworsley
Copy link
Contributor Author

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.

@stephenworsley stephenworsley marked this pull request as ready for review October 27, 2022 14:38
Copy link
Member

@lbdreyer lbdreyer left a 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/experimental/regrid_conservative.py Show resolved Hide resolved
lib/iris/analysis/_regrid.py Show resolved Hide resolved
lib/iris/analysis/_regrid.py Outdated Show resolved Hide resolved
lib/iris/analysis/_regrid.py Outdated Show resolved Hide resolved
lib/iris/analysis/_regrid.py Show resolved Hide resolved
lib/iris/analysis/_regrid.py Outdated Show resolved Hide resolved
@stephenworsley stephenworsley force-pushed the regridding_attempt branch 2 times, most recently from 613153c to a6c38f3 Compare November 10, 2022 11:11
Copy link
Member

@lbdreyer lbdreyer left a 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants