JiaweiZhuang / xESMF

Universal Regridder for Geospatial Data
http://xesmf.readthedocs.io/
MIT License
269 stars 49 forks source link

Adding ESMF.LocStream capabilities to xESMF #81

Closed raphaeldussin closed 4 years ago

raphaeldussin commented 4 years ago

@JiaweiZhuang this PR allows xESMF to use LocStream objects (sequence of geographical points). This is particularly useful to interpolate onto ocean sections, creating open boundary conditions,...

I have made minimal inserts, that do not change any default behavior. Also added some testing. Let me know if there's more to be done for the PR.

Best, Raf

codecov-io commented 4 years ago

Codecov Report

Merging #81 into master will increase coverage by 0.78%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #81      +/-   ##
==========================================
+ Coverage   95.76%   96.55%   +0.78%     
==========================================
  Files           6        6              
  Lines         260      319      +59     
==========================================
+ Hits          249      308      +59     
  Misses         11       11
Impacted Files Coverage Δ
xesmf/frontend.py 95.58% <100%> (+1.59%) :arrow_up:
xesmf/backend.py 96.51% <100%> (+0.51%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7479110...b84f2d7. Read the comment docs.

JiaweiZhuang commented 4 years ago

Thanks very much for the PR! This is something that I've been wanting to add but never got time to actually do it. Also thanks for the unit test. What would be even more useful is an example notebook under doc/notebooks to show a use case.

I am mostly thinking about how to have a consistent API to handle various cases:

Options are:

  1. Add locstream_out and locstream_in kwarg to Regridder, following your PR
  2. Have a new subclass LocStreamRegridder to handle locstream input/output, and avoid touching the original Regridder API. It might feel like different classes under sklearn linear regressors
  3. Mark the locstream attribute in the grid object ds_in / ds_out (detect coordinate name like locations, but it won't work for dict), and avoid additional kwargs or classes.

I was leaning towards no. 2, but your approach also makes sense.

JiaweiZhuang commented 4 years ago

Another complication from LocStream is that this class can only be used for:

It cannot be used for conservative method, for either source or destination.

Ref: ESMF Regridding Update

raphaeldussin commented 4 years ago

About the more general comments:

I went for option 1 because that's in my opinion, the one that added the less code. Once the ESMF Fields are created, ESMF Regridder works the same (not allowing all types of regridding of course). I think adding a locstream_in a way that's symetric to locstream_out should not be a big issues if you think that might be useful (see previous point). Even with LocStream in, I think 95+% of users will be coming for the grid-> grid interpolation. What do you think?

I'm gonna put some more tests, the codecov for my PR is not very good. An example notebook with follow once we ironed out the API details.

JiaweiZhuang commented 4 years ago

I think adding a locstream_in a way that's symetric to locstream_out should not be a big issues if you think that might be useful

Yeah I think it is good to have both locstream_out and locstream_in as options. The latter is useful for aligning observation data to model grid (similar to supersampling).

For the example notebook, it can be with synthetic data replicating advanced interpolation or with real data from xr.tutorial. Can revert the resulting locstream back to model grid via locstream_in.

raphaeldussin commented 4 years ago

@JiaweiZhuang sorry for the repeated commits, it's ready to be reviewed/merged.

raphaeldussin commented 4 years ago

@JiaweiZhuang I realized that I have missed one of your comments. Fixed now and I also expanded the test_build_regridder to include locstream_in.

Let me know if there's more that needs to be done. I'd really like to move fwd with it.

JiaweiZhuang commented 4 years ago

@raphaeldussin Sorry I had a long backlog. A few more comments on the test code, and it should be good to go.

raphaeldussin commented 4 years ago

move the locstream tests into their own functions.

JiaweiZhuang commented 4 years ago

Thanks @raphaeldussin! Added to 0.3.0 release.