Introduce `path_to_shapely` and `shapely_to_path` #2455

rcomer · 2024-10-15T16:30:52Z

Rationale

Closes #2447. Introduce shapely_to_path which maps a single Shapely geometry or collection to a single Matplotlib path, and path_to_shapely, which does the opposite. To take full advantage of this in our transforms code, also modify project_geometry so that it returns a GeometryCollection from either a GeometryCollection or a LinearRing. Previously GeometryCollection was not supported and LinearRing was mapped to a tuple.

Note the first commit just duplicates the existing path_to_geos and geos_to_path and all the material changes are in the second commit. I rearranged the logic in path_to_shapely because it seemed simpler to separate the geometry types into different lists up front.

I also went ahead and started a whatsnew for the next release. I think that was suggested a couple of releases ago and it does seem simpler to me to add the entries as we go.

Implications

The new behaviour of project_geometry from a LinearRing could be a breaking change but, since the docstring of project_geometry states that it returns a Shapely geometry, I claim this one as a bug fix.

rcomer · 2024-10-15T16:39:12Z

lib/cartopy/mpl/patch.py

-        if geom.is_empty:
-            pass


These 2 lines were introduced at #560, but the test added then has since been updated to expect a list of points. So I don't think this loop is relevant any more.

rcomer · 2024-10-15T16:39:39Z

lib/cartopy/mpl/patch.py

+    # Figure out what type of object to return
+    if not polygons:
+        if not linestrings:
+            if not points:
+                # No geometries.  Return an empty point
+                return sgeom.Point()
+            elif len(points) > 1:
+                return sgeom.MultiPoint(points)
+            else:
+                return points[0]
+        elif not points:
+            if len(linestrings) > 1:
+                return sgeom.MultiLineString(linestrings)
+            else:
+                return linestrings[0]
+    else:
+        if not linestrings and not points:
+            if len(polygons) > 1:
+                return sgeom.MultiPolygon(polygons)
+            else:
+                return polygons[0]


I feel like I'm missing an obvious slicker way to do this...

There is a lot of logic encapsulated here, so I think you might only be able to get it slightly simpler.

Maybe you could put these cases into a pattern matching syntax if we are only supporting 3.10+ for the next release... That might not be any easier/cleaner, so just throwing that out there as a possible idea. I think this looks OK currently.

rcomer · 2024-10-15T16:41:02Z

lib/cartopy/tests/mpl/test_caching.py

@@ -122,7 +122,8 @@ def test_contourf_transform_path_counting():
    gc.collect()
    initial_cache_size = len(cgeoaxes._PATH_TRANSFORM_CACHE)

-    with mock.patch('cartopy.mpl.patch.path_to_geos') as path_to_geos_counter:
+    with mock.patch('cartopy.mpl.patch.path_to_shapely',
+                    wraps=cartopy.mpl.patch.path_to_shapely) as path_to_shapely_counter:


This was failing because project_geometry received a MagicMock that it didn't know what to do with. I'm confused why that didn't also happen previously with path_to_geos.

rcomer · 2024-10-15T16:46:11Z

I also tested this with my user code that generates many contour plots. I got identical results with my branch and with main.

rcomer · 2024-10-15T16:49:42Z

Broke the minimum versions tests 😢

rcomer · 2024-10-15T18:05:49Z

Most of the tests were fixed by bumping minimum Shapely to v2.0. I think that’s reasonable to do since v2 will be 2 years old in December. However, I think the remaining test failure might need a workaround for lack of matplotlib/matplotlib#25252. It’s too soon to drop support for MPL 3.7.

docs/source/whatsnew/v0.25.rst

lib/cartopy/crs.py

lib/cartopy/mpl/patch.py

greglucas · 2024-10-16T15:38:37Z

lib/cartopy/mpl/patch.py

+        :class:`shapely.geometry.collection.GeometryCollection`.
+
+    """
+    # Convert path into numpy array of vertices (and associated codes)


Are we able to leverage any of the recent contour logic / path handling you were updating in Matplotlib?

https://github.com/matplotlib/matplotlib/blob/25a05f2399d6b63bbaad5a9380a8a89cec821c50/lib/matplotlib/contour.py#L795

There are a bunch of path handling routines for stitching the previous individual paths into a compound path with the recent refactor that might give some inspiration here as well.

ContourSet's allsegs and allkinds use the private Path._iter_connected_components. We could raise a feature request to have that made public. However, I made a quick test swapping out our path splitting logic here for _iter_connected_components and got some spectacular image test failures. I didn't dig too deep, but it seems some of the _iter_connected_components paths have an extra STOP on the end which our own splitting logic doesn't produce.

Or have I misunderstood what you meant?

That is exactly what I was wondering, thanks for looking into it :) I'm satisfied to keep it here for now and we can always move to something shared later.

I wonder if the extra STOPs result from an extra LINETO in the simplified path creation in MPL, noticed here and in the linked issues:
matplotlib/matplotlib#28478 (comment)
But that is a digression, this works great for now 👍

greglucas · 2024-10-16T15:47:06Z

lib/cartopy/mpl/patch.py

+    # Figure out what type of object to return
+    if not polygons:
+        if not linestrings:
+            if not points:
+                # No geometries.  Return an empty point
+                return sgeom.Point()
+            elif len(points) > 1:
+                return sgeom.MultiPoint(points)
+            else:
+                return points[0]
+        elif not points:
+            if len(linestrings) > 1:
+                return sgeom.MultiLineString(linestrings)
+            else:
+                return linestrings[0]
+    else:
+        if not linestrings and not points:
+            if len(polygons) > 1:
+                return sgeom.MultiPolygon(polygons)
+            else:
+                return polygons[0]


There is a lot of logic encapsulated here, so I think you might only be able to get it slightly simpler.

Maybe you could put these cases into a pattern matching syntax if we are only supporting 3.10+ for the next release... That might not be any easier/cleaner, so just throwing that out there as a possible idea. I think this looks OK currently.

lib/cartopy/mpl/patch.py

greglucas · 2024-10-16T15:52:02Z

lib/cartopy/mpl/patch.py

+            if _MPL_38 or path.vertices.size > 0:
+                paths.append(path)
+        return Path.make_compound_path(*paths)
+    elif hasattr(shape, '_as_mpl_path'):


What shapes/objects have _as_mpl_path that we haven't handled above? This seems a bit odd to have the function shapely_to_path but then accept other things besides shapely geometries.

greglucas · 2024-10-16T15:54:57Z

lib/cartopy/tests/mpl/test_patch.py

 import shapely.geometry as sgeom

 import cartopy.mpl.patch as cpatch


-class Test_path_to_geos:
-    def test_empty_polygon(self):
+@pytest.mark.parametrize('legacy', [False, True])


Maybe be a bit more explicit in the variable name about what is legacy here.

Suggested change

@pytest.mark.parametrize('legacy', [False, True])

@pytest.mark.parametrize('use_legacy_path_to_geos', [False, True])

rcomer · 2024-10-20T14:12:15Z

docs/source/whatsnew/v0.25.rst

+  deprecated.  Use `~cartopy.mpl.path.path_to_shapely` and
+  `~cartopy.mpl.path.shapely_to_path` instead.
+
+* `cartopy.mpl.patch.path_segments` has moved to `cartopy.mpl.path.path_segments`.


I'm wondering if we actually want to privatise path_segments? Since it's only two lines it's not hard for downstream to vendor, and it would mean we have more freedom to do what we want with it in future.

Yeah, I think it would be good to do this and it would allow us to use MPL's version in the future easier as a drop-in replacement.

rcomer · 2024-10-20T14:14:58Z

lib/cartopy/tests/mpl/test_path.py

@@ -0,0 +1,92 @@
+# Copyright Crown and Cartopy Contributors


I used git mv to change this from test_patch.py so I'm not clear why it's showing as a deletion and new file 😕

I've had this happen before as well and I think it is due to gits # of lines changed detection limits for what it decides is a move vs change, but I'm not positive and also curious.

I moved the rename to its own commit, so it's now easier to see what I actually changed. It also restored the blame that had gone missing, so I propose to keep that separate commit at merge.

rcomer · 2024-10-20T14:21:04Z

lib/cartopy/mpl/patch.py

+        :class:`shapely.geometry.collection.GeometryCollection`.
+
+    """
+    # Convert path into numpy array of vertices (and associated codes)


ContourSet's allsegs and allkinds use the private Path._iter_connected_components. We could raise a feature request to have that made public. However, I made a quick test swapping out our path splitting logic here for _iter_connected_components and got some spectacular image test failures. I didn't dig too deep, but it seems some of the _iter_connected_components paths have an extra STOP on the end which our own splitting logic doesn't produce.

Or have I misunderstood what you meant?

rcomer commented Oct 15, 2024

View reviewed changes

rcomer marked this pull request as draft October 15, 2024 16:51

rcomer force-pushed the path-shapely branch from 5a253f0 to f6b65e7 Compare October 15, 2024 18:51

rcomer marked this pull request as ready for review October 15, 2024 19:00

rcomer added Component: Geometry transforms API Change Denotes issues or PRs that change the interface labels Oct 15, 2024

greglucas reviewed Oct 16, 2024

View reviewed changes

rcomer added 2 commits October 20, 2024 13:32

Bump Shapely minimum version to 2.0

cac5209

Duplicate patch module

5ba97d1

rcomer force-pushed the path-shapely branch from f6b65e7 to b53ab7f Compare October 20, 2024 14:08

rcomer commented Oct 20, 2024

View reviewed changes

rcomer force-pushed the path-shapely branch from b53ab7f to f8b59ed Compare October 20, 2024 16:30

greglucas approved these changes Oct 21, 2024

View reviewed changes

rcomer added 2 commits October 21, 2024 19:40

Implement path_to_shapely and shapely_to_path

8bb65fb

TST: renamed test_patch.py to test_path.py

b64d700

rcomer force-pushed the path-shapely branch from f8b59ed to b64d700 Compare October 21, 2024 18:42

greglucas merged commit 6b5e619 into SciTools:main Oct 23, 2024
23 checks passed

rcomer deleted the path-shapely branch October 23, 2024 19:47

greglucas mentioned this pull request Oct 23, 2024

fix: Invalid Polygon from Contouring (Issue #2370) #2373

Closed

QuLogic added this to the Next Release milestone Oct 23, 2024

greglucas mentioned this pull request Oct 24, 2024

ENH: Use shapefile crs attribute when possible #2307

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `path_to_shapely` and `shapely_to_path` #2455

Introduce `path_to_shapely` and `shapely_to_path` #2455

rcomer commented Oct 15, 2024

rcomer Oct 15, 2024 •

edited

Loading

rcomer Oct 15, 2024

greglucas Oct 16, 2024

rcomer Oct 15, 2024

rcomer commented Oct 15, 2024

rcomer commented Oct 15, 2024 •

edited

Loading

rcomer commented Oct 15, 2024

greglucas Oct 16, 2024

rcomer Oct 20, 2024

greglucas Oct 21, 2024

greglucas Oct 16, 2024

greglucas Oct 16, 2024

greglucas Oct 16, 2024

rcomer Oct 20, 2024

greglucas Oct 21, 2024

rcomer Oct 20, 2024

greglucas Oct 21, 2024

rcomer Oct 21, 2024

rcomer Oct 20, 2024

	@pytest.mark.parametrize('legacy', [False, True])
	@pytest.mark.parametrize('use_legacy_path_to_geos', [False, True])

Introduce path_to_shapely and shapely_to_path #2455

Introduce path_to_shapely and shapely_to_path #2455

Conversation

rcomer commented Oct 15, 2024

Rationale

Implications

rcomer Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcomer commented Oct 15, 2024

rcomer commented Oct 15, 2024 • edited Loading

rcomer commented Oct 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Introduce `path_to_shapely` and `shapely_to_path` #2455

Introduce `path_to_shapely` and `shapely_to_path` #2455

rcomer Oct 15, 2024 •

edited

Loading

rcomer commented Oct 15, 2024 •

edited

Loading