cf-python icon indicating copy to clipboard operation
cf-python copied to clipboard

dask: `cf.Field.regrids` and `cf.Field.regridc`

Open davidhassell opened this issue 3 years ago • 2 comments

I'm finished. In many ways.

Hopefully self explanatory ... might be easiest to start at cf.Field.regrid[sc] and work backwards from there.

I'll draw a bit of UML-like to outline the structure. Watch this space.

You need to run python create_test_files.py before testing for the first time.

Thanks!

davidhassell avatar Aug 22 '22 17:08 davidhassell

Hi Sadie, I don't want to prejudice your review, but as it's a big PR I'll take the liberty of making a suggestion, which you are of course wholly free to ignore:

Since the methodology of this PR is so different to what we had at v3.13.0, it may be better to ignore what we had before and just treat all this as "new" code. The new units tests are much more sensible than the old ones (i.e. the new tests actually test against the ESMF "truth", rather than an older execution of the cf code, which is what the old tests were gong on (sic!)); so, given that the new tests pass, I (at least) am fairly happy that it's all working ...

davidhassell avatar Aug 22 '22 17:08 davidhassell

image

davidhassell avatar Aug 22 '22 18:08 davidhassell

Thought I'd review myself to check all is well for your review. Just a few commits to spruce up the explanations.

davidhassell avatar Nov 14 '22 16:11 davidhassell

I've took the liberty of fixing the lone trivial merge conflict (one line of imports on each side, where I checked if the other from lama-to-dask was used still and it wasn't) just so I can submit my final review shortly on the exact ready-to-merge state of the code.

sadielbartholomew avatar Jan 30 '23 15:01 sadielbartholomew

Addressing the general minor comments:

  1. test_Field references one of the old regrid_fileN.ns files (N=1)

We'll open a new PR for this.

  1. I'm getting a docstring substitution error raised via the test.

Sorted: https://github.com/NCAS-CMS/cf-python/pull/438/commits/2ca3e2cf778461ee1b3aeac445f3adac03120467

  1. When I run test_regrid, I get repeated (hundreds of) deprecation warnings coming through from ESMF.

As discussed, Ankit and I don't :) We'll put it down to rogue versions and ignore.

  1. Some of the old RegridOperator methods that have been deleted here are still being listed in the documentation

Sorted: https://github.com/NCAS-CMS/cf-python/pull/438/commits/ee57d6e0c7531d68f3c3361baeade81e13ac8687

  1. Flake8 is reporting some errors.

Annoying, but we'll ignore for now. It's possible that when merged into lama-to-dask they'll get fixed, as this quite an old PR. Maybe!

davidhassell avatar Jan 30 '23 18:01 davidhassell

A very large thank you to Sadie for tackling the review of this rather large PR. Nice job.

davidhassell avatar Jan 30 '23 19:01 davidhassell

(I've since realised my main initial review comment got mangled, but understood and discussed offline anyway.)

sadielbartholomew avatar Jan 30 '23 19:01 sadielbartholomew