-
Notifications
You must be signed in to change notification settings - Fork 10
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 encode_cf, decode_cf #69
Conversation
This is cool! I am fine with the initial limit of one geometry coordinate as that would already result in a parity with R's Is there any blocker preventing this to move beyond POC? |
Test and docstrings. Do you want |
Given |
Boom with xarray-contrib/cf-xarray#526 xvec supports encoding/decoding multiple geometries. The roundtrip tests fail because of an extra It could use quite a bit of testing :) |
decoded = decoded.xvec.set_geom_indexes( | ||
dim, crs=crs.get(decoded[dim].attrs.get("grid_mapping", None)) | ||
) |
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.
This is the key buggy line. it always sets the index, we do not record which geometry dims were indexed at encode-time. What should we do here?
As an aside it'd be nice for set_geom_indexes
to understand the grid_mapping
convention. WDYT?
One approach: decode_cf
does NOT set the new index, but the user does so manually. Instead set_geom_indexes
learns how to interpret the grid_mapping
convention so CRS is set properly by default.
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.
it always sets the index, we do not record which geometry dims were indexed at encode-time. What should we do here?
Is that an issue if we just index all geom dims encoded in the file?
As an aside it'd be nice for set_geom_indexes to understand the grid_mapping convention. WDYT?
Not against but I don't really know what would it mean implementation-wise. Maybe just a simple call to pyproj.CRS.from_cf
?
set_geom_indexes learns how to interpret the grid_mapping convention so CRS is set properly by default.
That would be preferable. Not a fan of asking users to set indexes after reading what already was indexed before writing.
ds["geom"].attrs["crs"] = ds.xindexes["geom"].crs | ||
ds["geom_z"].attrs["crs"] = ds.xindexes["geom_z"].crs |
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.
Is there a reason you can't set these in GeometryIndex.create_variables
?
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.
Apart from "no one thought about that until now", I am not aware of any.
Given the crs information is now stored in the index itself, I guess we can just drop it? Not sure about consequences. |
Yes I don't see why you duplicate it. |
This reverts commit 2a7cf38.
What needs to happen here apart from merge of that PR in cf-xarray? Tests locally pass and apart from that commented out ValueError, we should probably raise when needed, I don't see anything obviously missing. |
Not much. I'd like to copy some tests over to cf-xarray. I'm on vacation right now so don't get to this for another week and a half |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #69 +/- ##
==========================================
- Coverage 99.29% 99.06% -0.24%
==========================================
Files 4 4
Lines 427 534 +107
==========================================
+ Hits 424 529 +105
- Misses 3 5 +2 ☔ View full report in Codecov by Sentry. |
OK Should be good to go for now. We could always keep tweaking but this should be great for experimenting and making it sure works with real-world datasets. |
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.
Thanks @dcherian!
xref #26, #48
This is a proof-of-concept really but it works for that demo notebook