Add conversion between cf and shapely for lines #460

jsignell · 2023-07-17T12:49:14Z

I started on this at the SciPy sprints with the idea of pushing forward the work described in this ticket: xarray-contrib/xvec#48. It took me a while to get the logic right 😅 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 · 2023-07-17T12:52:29Z

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 ⚠️

Additional details and impacted files

@@            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	Coverage Δ
mypy	`42.37% <26.92%> (-0.50%)`	⬇️
unittests	`94.11% <100.00%> (+0.20%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dcherian · 2023-07-17T13:22:50Z

Thanks! This is great to see

cc @aulemahal

jsignell · 2023-07-17T13:29:51Z

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.

jsignell · 2023-07-17T13:32:38Z

cf_xarray/tests/test_geometry.py

+            [0, 2, 4, 6, 0, 0, 1, 1.0, 2.0, 9.5], dims=("node",), attrs={"axis": "Y"}
+        ),
+        part_node_count=xr.DataArray([4, 3, 3], dims=("index",)),
+        node_count=xr.DataArray([2, 2, 3, 3], dims=("counter",)),


This part caught me up for a bit. As I understand it, the node_count is the number of nodes per line segment. So each line segment in a multiline results in a value in this array. This means that the dims is not index and is not in the shapely version at all. I gave it a kind of random name counter which should probably be changed.

cf_xarray/geometry.py

dcherian · 2023-10-25T03:06:11Z

@jsignell Any interest in picking this back up?

martinfleis · 2023-10-25T05:46:31Z

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 · 2023-10-26T14:11:18Z

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 · 2023-10-26T14:28:39Z

@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 · 2023-10-26T16:06:50Z

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 · 2023-11-03T21:35:02Z

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 · 2023-11-06T19:58:25Z

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 · 2023-11-06T20:55:48Z

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

jsignell · 2023-11-30T14:27:01Z

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

for more information, see https://pre-commit.ci

dcherian · 2023-12-01T18:28:26Z

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

martinfleis · 2023-12-01T18:45:46Z

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

jsignell · 2023-12-01T19:00:10Z

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

dcherian · 2023-12-11T04:34:24Z

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

jsignell · 2023-12-11T14:11:57Z

YOLO!

martinfleis

Approved (even if too late)! Looks good @jsignell!

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

jsignell · 2023-12-12T13:05:04Z

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 · 2023-12-12T15:50:45Z

Thanks for taking a look @martinfleis 🙏

jsignell marked this pull request as draft July 17, 2023 12:52

jsignell commented Jul 17, 2023

View reviewed changes

cf_xarray/geometry.py Outdated Show resolved Hide resolved

jsignell force-pushed the lines branch from 114173c to 9e12bdf Compare November 3, 2023 21:39

jsignell marked this pull request as ready for review November 6, 2023 20:56

dcherian requested review from aulemahal and martinfleis November 6, 2023 22:35

jsignell and others added 10 commits December 1, 2023 11:30

Add conversion between cf and shapely for lines

923d72e

Make polygons raise

1fcdf22

[pre-commit.ci] auto fixes from pre-commit.com hooks

06488ba

for more information, see https://pre-commit.ci

Switch to using linestrings and multilinestrings

1346464

[pre-commit.ci] auto fixes from pre-commit.com hooks

3e21f12

for more information, see https://pre-commit.ci

Add shapely to CF for lines

0b2e075

[pre-commit.ci] auto fixes from pre-commit.com hooks

dd93e37

for more information, see https://pre-commit.ci

Use from_ragged_array

1a474df

[pre-commit.ci] auto fixes from pre-commit.com hooks

94fe4b0

for more information, see https://pre-commit.ci

Fix mypy

6b228e1

jsignell force-pushed the lines branch from 13bff3e to 6b228e1 Compare December 1, 2023 16:30

dcherian and others added 4 commits December 1, 2023 10:41

Merge branch 'main' into lines

7541593

Full test coverage for patch

61fd246

[pre-commit.ci] auto fixes from pre-commit.com hooks

f9f00cd

for more information, see https://pre-commit.ci

Merge branch 'main' into lines

bea680c

dcherian merged commit 02b1d74 into xarray-contrib:main Dec 11, 2023
12 checks passed

jsignell deleted the lines branch December 11, 2023 14:11

martinfleis reviewed Dec 12, 2023

View reviewed changes

This was referenced Jan 15, 2024

Fix conversion between shapely and cf for lines #493

Merged

Add conversion between shapely and cf for polygons #495

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add conversion between cf and shapely for lines #460

Add conversion between cf and shapely for lines #460

jsignell commented Jul 17, 2023

codecov bot commented Jul 17, 2023 •

edited

Loading

dcherian commented Jul 17, 2023

jsignell commented Jul 17, 2023

jsignell Jul 17, 2023

dcherian commented Oct 25, 2023

martinfleis commented Oct 25, 2023

jsignell commented Oct 26, 2023

martinfleis commented Oct 26, 2023

jsignell commented Oct 26, 2023

jsignell commented Nov 3, 2023

jsignell commented Nov 6, 2023

jsignell commented Nov 6, 2023

jsignell commented Nov 30, 2023

dcherian commented Dec 1, 2023

martinfleis commented Dec 1, 2023

jsignell commented Dec 1, 2023

dcherian commented Dec 11, 2023

jsignell commented Dec 11, 2023

martinfleis left a comment

jsignell commented Dec 12, 2023

dcherian commented Dec 12, 2023

Add conversion between cf and shapely for lines #460

Add conversion between cf and shapely for lines #460

Conversation

jsignell commented Jul 17, 2023

codecov bot commented Jul 17, 2023 • edited Loading

Codecov Report

dcherian commented Jul 17, 2023

jsignell commented Jul 17, 2023

jsignell Jul 17, 2023

Choose a reason for hiding this comment

dcherian commented Oct 25, 2023

martinfleis commented Oct 25, 2023

jsignell commented Oct 26, 2023

martinfleis commented Oct 26, 2023

jsignell commented Oct 26, 2023

jsignell commented Nov 3, 2023

jsignell commented Nov 6, 2023

jsignell commented Nov 6, 2023

jsignell commented Nov 30, 2023

dcherian commented Dec 1, 2023

martinfleis commented Dec 1, 2023

jsignell commented Dec 1, 2023

dcherian commented Dec 11, 2023

jsignell commented Dec 11, 2023

martinfleis left a comment

Choose a reason for hiding this comment

jsignell commented Dec 12, 2023

dcherian commented Dec 12, 2023

codecov bot commented Jul 17, 2023 •

edited

Loading