fix treatment of spine paths with explicit path-codes

[raphaelquast]

Rationale

The current implementation of the adjust_location method of GeoSpines assumes that the spine-path is a single connected path and potential path-codes are ignored.

This causes problems if multi-paths are used as bounaries to clip the map (e.g. with ax.set_boundary().... for example:

Implications

With the proposed changes, path-codes are preserved and multi-path clipping works as expected

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[raphaelquast]

@greglucas
I saw that you initially introduced this change a few months ago to avoid

"... a small gap at the corner of closure due to the capstyle of the lines being "butt" by default".

To keep your changes for single closed paths, I've implemented the fix so that it only triggers if the path actually represents a multi-path (e.g. if it contains more than 1 MOVETO code).

[raphaelquast]

Tests fail with HTTP Error 502: Bad Gateway... I guess they require a re-run?

[greglucas]

Is there a way to do something like: if not path.closed(): close_the_path

One other thought is that perhaps the previous fix was actually a non-ideal one where self._path = mpath.Path(self._path.vertices, closed=True) is going to remove the final vertex from the path, but I think we actually might want to append the beginning vertex to the end and then close so we don't drop a point?

This should get a test somehow, can you modify your example figure output to count moveto/close before/after this change?

[raphaelquast]

Hey, @greglucas, thanks for the quick response!

One other thought is that perhaps the previous fix was actually a non-ideal one where self._path = mpath.Path(self._path.vertices, closed=True) is going to remove the final vertex from the path, but I think we actually might want to append the beginning vertex to the end and then close so we don't drop a point?

I had a quick look... it seems that using Path(..., closed=True) actually just replaces all path-codes with this:

...
elif closed and len(vertices):
    codes = np.empty(len(vertices), dtype=self.code_type)
    codes[0] = self.MOVETO
    codes[1:-1] = self.LINETO
    codes[-1] = self.CLOSEPOLY

I agree that this might cause problems if the last vertex and the first should actually be connected.
CLOSEPOLY is described in the docs as:

- CLOSEPOLY: 1 vertex (ignored)
   Draw a line segment to the start point of the current polyline.

I've now updated the PR to perform a more complete check and make sure that the path (and all potential sub-paths) are properly closed with the start-vertex.

---

This should get a test somehow, can you modify your example figure output to count moveto/close before/after this change?

For the currently implemented method I don't think it's necessary to count since Path(..., closed=True) will simply replace all codes as indicated above.

If you agree with my proposed solution, I can write a test for GeoSpine._ensure_path_closed() that checks some potential use-cases (e.g. treatment of single closed path, single open path, multi-paths etc.).

[raphaelquast]

Test-failure is again just a download-issue.

[greglucas]

@raphaelquast , some tests would be good regardless of the solution I think! You've definitely found some issues, so codifying those with tests would be great.

I think this is good. I do have some concern that this is in a potential hot-loop when zooming/panning around, but the fix looks like the "proper" solution to account for the various cases.

[raphaelquast]

@greglucas I've now added some basic unittests to check proper handling of single- and multi-path boundaries (based on image-comparison)

[raphaelquast]

Test failures are again unrelated but I don't seem to have the rights to trigger a manual re-run.

[raphaelquast]

Hey, just checking in to see what's missing to get this merged?

• Test failures after re-run still don't relate to this PR but to WFS connection issues)
• I've contributed to cartopy before... as far as I remember I've already signed the CLA back then... do you need me to re-sign it?

[greglucas]

Sorry, been really busy on other projects the past month and lost track of this, thanks for the ping!

CLA
Yes, you should sign it again. There is a new CLA on all SciTools contributions now. It is a much simpler process and should take place immediately instead of needing a human to review it.

Code
Did you look at this as an option for closing the polygons? https://matplotlib.org/stable/api/path_api.html#matplotlib.path.Path.to_polygons
Specifically, this comment catches my eye saying any unclosed polys will be closed:

If closed_only is True (default), only closed polygons, with the last point being the same as the first point, will be returned. Any unclosed polylines in the path will be explicitly closed.

Testing
Are you able to put all of those tests into a single image with subplots to reduce the number of image comparisons/binary data you're adding?

Is there a way to test this without image comparisons. Our image comparisons are always fragile, so I'm wondering if you can say something like "before this change the boundary path had only 1 CLOSEPOLY, now it should have 5".