-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add IndexRaster for gridding on reduced weighted rasters #10
Conversation
Ok, was able to fix the failing cf_xarray import by bumping the python version to 3.9, but there are some regression tests failures i am not getting to the bottom of today. |
Hi @coroa - I will do a more thorough review when I have time later today. But I just ran the tests locally and all pass - so have restarted the job here. |
Hi @coroa I have now reviewed and can only admire the approach and implementation. Let me know when you have a chance to update the docs and add tests, which will also help me do a full review. |
Hey @coroa I just realized here that everything in this PR is a pure addition and no existing code is touched. So I have to say I'm really confused as to why the tests are failing (again noting they pass on my local machine). |
Global proxies are NOT sums of regional ones (since they can can also contain ocean areas), thus `to_total` invites confusion.
Due to a typing incompatibility in cf_xarray
rebased on main |
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #10 +/- ##
==========================================
- Coverage 76.73% 68.25% -8.49%
==========================================
Files 2 2
Lines 245 315 +70
Branches 57 74 +17
==========================================
+ Hits 188 215 +27
- Misses 26 69 +43
Partials 31 31 ☔ View full report in Codecov by Sentry. |
Use `compute` on indexraster instance instead.
Current test failures is because of
|
Ready for review! And merging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great @coroa - I did a scan of the doc notebook and tests, which all make sense to me. Thanks so much for this!
IndexRaster holds (and is able to serialize) a reduced weighted raster, ie. one that is split in:
indicator
, where integer values from 1 to number of shapes indicate cells fully belonging to a single shapeboundary
made up of the weights similar to the previous weighted raster (but only covering cells actually on the boundary)The
boundary
uses a MultiIndex calledspatial
as a sparse representation.aggregate
andgrid
methods allow to aggregate gridded data and to grid indexed data.There is a
from_weighted_raster
constructor which transforms a weighted raster into the reduced form. It would be good to add a rasterization strategy similar to maybehybrid
which directly creates anIndexRaster
.Documentation and testing has not been addressed yet.
Two new dependencies need to be pulled in (we could also make them optional?):
flox
is necessary for aggregating data from the grid efficientlycf_xarray
provides two convenience functions which allow serializing and deserializing multi-index coordinates