SciTools / iris-esmf-regrid

A collection of structured and unstructured ESMF regridding schemes for Iris.
https://iris-esmf-regrid.readthedocs.io/en/latest
BSD 3-Clause "New" or "Revised" License
19 stars 17 forks source link

Support building regridders with masks #219

Closed stephenworsley closed 1 year ago

stephenworsley commented 1 year ago

Addresses #218 by having ESMF ignore masked points.

TODO

codecov[bot] commented 1 year ago

Codecov Report

Merging #219 (b309b4a) into main (38f2b67) will increase coverage by 0.02%. The diff coverage is 98.79%.

:exclamation: Current head b309b4a differs from pull request most recent head e06c20e. Consider uploading reports for the commit e06c20e to get more accurate results

@@            Coverage Diff             @@
##             main     #219      +/-   ##
==========================================
+ Coverage   99.19%   99.22%   +0.02%     
==========================================
  Files          28       28              
  Lines        2871     3095     +224     
==========================================
+ Hits         2848     3071     +223     
- Misses         23       24       +1     
Impacted Files Coverage Δ
esmf_regrid/experimental/unstructured_regrid.py 97.36% <ø> (ø)
esmf_regrid/_esmf_sdo.py 96.51% <95.23%> (+1.02%) :arrow_up:
esmf_regrid/schemes.py 97.43% <97.26%> (-0.35%) :arrow_down:
esmf_regrid/esmf_regridder.py 95.45% <100.00%> (+0.06%) :arrow_up:
esmf_regrid/experimental/io.py 100.00% <100.00%> (ø)
esmf_regrid/experimental/unstructured_scheme.py 98.56% <100.00%> (+0.05%) :arrow_up:
.../tests/unit/experimental/io/test_round_tripping.py 100.00% <100.00%> (ø)
...nstructured_scheme/test_GridToMeshESMFRegridder.py 100.00% <100.00%> (ø)
...nstructured_scheme/test_MeshToGridESMFRegridder.py 100.00% <100.00%> (ø)
...sts/unit/schemes/test_ESMFAreaWeightedRegridder.py 100.00% <100.00%> (ø)
... and 1 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

github-actions[bot] commented 1 year ago

@SciTools-incubator/esmf-regrid-devs This pull-request is stale due to a lack of activity in the last 90 days. Remove stale label or comment, otherwise this pull-request will close automatically in 7 days time.

github-actions[bot] commented 1 year ago

@SciT@SciTools-incubator/esmf-regrid-devs This stale pull-request has been automatically closed due to no community activity

stephenworsley commented 1 year ago

The benchmarks run against a curvilinear grid with a row of degenerate cells which would cause the regridder to run slower. masking. Benchmarks are run with and without the mask being included. When the mask is included there are significant performance improvements (this may be even better when the cells are particularly bad). Note that this benchmarks with masks fails on main. image

trexfeathers commented 1 year ago

Apologies, I know I can get a bit evangelical about D.R.Y. code, but do I feel this PR makes a very strong case for more effort in this space.

I believe I have the second-most experience with this codebase, but all the layers and parallel changes made me get quite lost during review. I'm also going to find it difficult to confirm which of my suggestions have been actioned, as the action will need to be mirrored in an uncertain number of places. Also, if I wanted to make an enhancement like this, I doubt I would be successful in remembering all the different places a change needed to be made.

D.R.Y. takes longer to get working, and I appreciate that's work that could otherwise be spent on new features, but it makes future work quicker, easier and less error-prone. This is all doubly true when it comes to developers who are not the original author. I'm basically describing Technical Debt. I know there are already some ideas in the pipeline (#198); take this as my vote for expediting that as well as going further; let's pay off that debt sooner before we pay much more Interest 👍.

Cc @pp-mo as the buddy for iris-esmf-regrid.

valeriupredoi commented 1 year ago

just plopping myself here to say hi and good work on this, guys! Looking forward to see this and the other features, including the handling of the new esmpy in an upcoming release :beers:

stephenworsley commented 1 year ago

This should have a CHANGELOG entry:

I've decided to add an entry for #241 while I was there.