xarray-contrib / xvec

Vector data cubes for Xarray
https://xvec.readthedocs.io
MIT License
93 stars 9 forks source link

Add encode_cf, decode_cf #69

Closed dcherian closed 1 month ago

dcherian commented 2 months ago

xref #26, #48

This is a proof-of-concept really but it works for that demo notebook

encoded = cube.xvec.encode_cf()
encoded.to_zarr("cube.zarr")
roundtripped = xr.open_zarr("cube.zarr").xvec.decode_cf()
roundtripped.identical(cube)  # True
martinfleis commented 2 months ago

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 {stars} implementation (I think, it may not be entirely up to date).

Is there any blocker preventing this to move beyond POC?

dcherian commented 2 months ago

Is there any blocker preventing this to move beyond POC?

Test and docstrings. Do you want cf_xarray as a required or optional dependency?

martinfleis commented 2 months ago

Do you want cf_xarray as a required or optional dependency?

Given cf_xarray itself depends on xarray only, I think it is fine to depend on it directly.

dcherian commented 2 months ago

Boom with https://github.com/xarray-contrib/cf-xarray/pull/526 xvec supports encoding/decoding multiple geometries.

The roundtrip tests fail because of an extra crs attribute added in decode_cf. Can you help me fix that? Should we be deleting that attribute?

It could use quite a bit of testing :)

martinfleis commented 2 months ago

The roundtrip tests fail because of an extra crs attribute added in decode_cf. Can you help me fix that? Should we be deleting that attribute?

Given the crs information is now stored in the index itself, I guess we can just drop it? Not sure about consequences.

dcherian commented 2 months ago

Yes I don't see why you duplicate it.

martinfleis commented 2 months ago

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.

dcherian commented 2 months ago

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[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.06%. Comparing base (c2260b8) to head (c88ea9c). Report is 2 commits behind head on main.

Additional details and impacted files ```diff @@ 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 ```

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

dcherian commented 1 month ago

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.