opendatacube / odc-geo

GeoBox and geometry utilities extracted from datacube-core
https://odc-geo.readthedocs.io/en/latest/
Apache License 2.0
83 stars 15 forks source link

Add new `odc.mask` and `odc.crop` methods for masking and cropping `xarray` data by geometries #114

Closed robbibt closed 10 months ago

robbibt commented 11 months ago

This PR introduces a new .odc.crop method that supports:

image

This will be very useful for pixel drill workflows that regularly involve a sequence of loading data, cropping to a certain extent using .sel(), then rasterizing a polygon and applying it as a mask.

I've tested that this works on both datasets and data arrays, and have included a check that the polygon overlaps with the geobox extent. Would welcome any other feedback to make this more robust!

codecov[bot] commented 11 months ago

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (eb12c48) 98.12% compared to head (40330a7) 97.91%. Report is 12 commits behind head on develop.

Files Patch % Lines
odc/geo/_xr_interop.py 88.37% 5 Missing :warning:
odc/geo/overlap.py 33.33% 4 Missing :warning:
odc/geo/geobox.py 95.12% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #114 +/- ## =========================================== - Coverage 98.12% 97.91% -0.22% =========================================== Files 25 25 Lines 4224 4315 +91 =========================================== + Hits 4145 4225 +80 - Misses 79 90 +11 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

github-actions[bot] commented 11 months ago

🚀 Deployed on https://658f9d0bfef25b2b78f663a8--odc-geo-docs.netlify.app

Kirill888 commented 11 months ago

This is great, thanks Robbi.

Can you please add this method to docs, here:

https://github.com/opendatacube/odc-geo/blob/6ce7524b6808457418b2c8bcecf01c918ce6458a/docs/api.rst?plain=1#L39-L52

Kirill888 commented 11 months ago

I think it might be better to expose 2 methods:

@robbibt also when happy with changes please rebase it all to a single commit:

  1. git reset --soft HEAD~<number of commits to merge into one>
  2. commit everything again as a single change this time
  3. push to github: git push --force-with-lease
robbibt commented 11 months ago

I think it might be better to expose 2 methods:

  • .mask(geom, ...options) -> xr.DataArray | xr.Dataset returning masked result in the source geobox
  • .crop(geom, ...options) -> xr.DataArray | xr.Dataset returning cropped and possibly masked result

@robbibt also when happy with changes please rebase it all to a single commit:

  1. git reset --soft HEAD~<number of commits to merge into one>
  2. commit everything again as a single change this time
  3. push to github: git push --force-with-lease

I agree, both a mask and crop method would be useful. I'll have another shot soon and will rebase everything after.

robbibt commented 10 months ago

@robbibt thanks for this, I think we should merge this as is, we can do further interface polishing, before next release.

Thanks @Kirill888 - should I combine the commits into one again? And what merge option do you use here?

image

Kirill888 commented 10 months ago

I usually go with Rebase and merge for cleaner history