xarray-contrib / cf-xarray

an accessor for xarray objects that interprets CF attributes
https://cf-xarray.readthedocs.io/
Apache License 2.0
157 stars 39 forks source link

Add conversion between cf and shapely for lines #460

Closed jsignell closed 9 months ago

jsignell commented 1 year ago

I started on this at the SciPy sprints with the idea of pushing forward the work described in this ticket: https://github.com/xarray-contrib/xvec/issues/48. It took me a while to get the logic right :sweat_smile: so I wanted to make sure it was somewhere public in case I don't get a chance to come back to it in the near future.

codecov[bot] commented 1 year ago

Codecov Report

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

Comparison is base (2648109) 83.10% compared to head (bea680c) 83.26%.

Files Patch % Lines
cf_xarray/geometry.py 89.74% 7 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #460 +/- ## ========================================== + Coverage 83.10% 83.26% +0.15% ========================================== Files 14 14 Lines 2676 2749 +73 Branches 189 199 +10 ========================================== + Hits 2224 2289 +65 - Misses 402 409 +7 - Partials 50 51 +1 ``` | [Flag](https://app.codecov.io/gh/xarray-contrib/cf-xarray/pull/460/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=xarray-contrib) | Coverage Δ | | |---|---|---| | [mypy](https://app.codecov.io/gh/xarray-contrib/cf-xarray/pull/460/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=xarray-contrib) | `42.37% <26.92%> (-0.50%)` | :arrow_down: | | [unittests](https://app.codecov.io/gh/xarray-contrib/cf-xarray/pull/460/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=xarray-contrib) | `94.11% <100.00%> (+0.20%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=xarray-contrib#carryforward-flags-in-the-pull-request-comment) to find out more.

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

dcherian commented 1 year ago

Thanks! This is great to see

cc @aulemahal

jsignell commented 1 year ago

Just to make sure this is visible from this side as well: while working on this PR I was pointed to https://github.com/twhiteaker/CFGeom. I have opened an issue on that repo to see if the contributors over there have opinions about how these efforts relate to each other.

dcherian commented 11 months ago

@jsignell Any interest in picking this back up?

martinfleis commented 11 months ago

This could be significantly simplified by using shapely.to_ragged_arrays and from_ragged_arrays. That is used to create a GeoArrow arrays of coordinates, which are nearly the same as CF representation. Just one uses offsets to indicate where a new geom starts and the others counts of coords per geom, which should be an easy conversion.

jsignell commented 11 months ago

The motivation for this work was to support IO in xvec, so I kind of stalled after @martinfleis came up with a better solution on that side. If there is still desire for this work then sure I can pick it back up.

martinfleis commented 11 months ago

@jsignell if you mean the WKB option, there is a bit of disagreement if that is a good idea. If we can get conversion to CF geometries via ragged array IO shapely has, we can use practically the same for GeoArrow option and Zarr (if we decide that is what we want). I still like the elegance of WKB but understand that some folks don't like saving binary objects into Zarr.

jsignell commented 11 months ago

Ah yeah I did mean the WKB option. In that case I can pick this back up. Or @dcherian feel free to take it over if you have some urgency around it.

jsignell commented 11 months ago

Thanks for the tip @martinfleis! I looked into from_ragged_array and I agree that it is pretty similar. Actually the logic that is in this PR for creating linestrings is nearly identical: https://github.com/shapely/shapely/blob/9540b77a939ba35d066d618183e67fc905ed9ec4/shapely/_ragged_array.py#L334-L337

I think the complexity in this PR comes from how cf intermingles MULTILINESTRING and LINESTRING. So once you have the line segments you still need to figure out which of them are going to be MULTILINESTRING objects. That is basically what the entirety this section is doing: https://github.com/xarray-contrib/cf-xarray/pull/460/files#diff-6484364cf7fa3216828551b2531094b30d77cf393313226a01d425c22580688aR353-R370

That is probably not the best possible way to do that conversion but I don't see anything similar in the shapely.from_ragged_array implementation

jsignell commented 11 months ago

Ok I added the shapely to CF direction for that one I found it pretty straightforward to use to_ragged_array so that one should read pretty cleanly! I'm going to try again now to see if I can simplify the other direction using from_ragged_array

jsignell commented 11 months ago

Ok this is clearly a case of Monday brain > Friday brain.

jsignell commented 10 months ago

I'm going to see what I can do to fix CI and improve coverage.

dcherian commented 10 months ago

I can't really review this so I'm happy to merge when you're ready.

martinfleis commented 10 months ago

I'll do the review, it is on my to-do list with an increasing priority :D.

jsignell commented 10 months ago

Yeah it's finally green and fairly well tested so it is actually ready for review for the first time :sweat_smile:

dcherian commented 9 months ago

Thanks @jsignell . We can address @martinfleis comments as they come in :)

jsignell commented 9 months ago

YOLO!

jsignell commented 9 months ago

I suppose that Polygons will look similar but we'll need to deal with holes as one layer on top of this.

Yeah I will probably start on that one now with all this fresh in my head.

Also I don't think I really considered z, so that might be something to look out for.

dcherian commented 9 months ago

Thanks for taking a look @martinfleis 🙏