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]

All committers have signed the CLA.

[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".

[greglucas]

@raphaelquast friendly ping in case you had a chance to look into to_polygons() and had any thoughts on the other comments.

[raphaelquast]

Hey @greglucas
I have not forgotten about this, but life has its way to get between me and my open-source endeavors 😅

I had a quick look so far but nothing concrete. In general to_polygons() looks like a good idea, but I wanted to do some testing first to make sure it does what we want it to do. I hope that I'll find some time in the next few weeks to look into this a bit further.

Concerning the tests:
I went for an image-comparison test since I thought that it will also catch any changes result in connection-lines between multi-polygon spines. Since there is no text involved, I expected the image comparisons to be stable.

Its of course possible to check if the updated polygon-representation contains enough CLOSEPOLY codes (or something similar). However, I would argue that such a test should be applied to the to_polygons function directly and not to some derived cartopy functionality like ax.set_boundary... what I wanted to test here is if the associated spines are drawn correctly.

[raphaelquast]

Hey @greglucas
I finally managed to do a quick check using the following implementation of the Path.to_polygons() method:

    @staticmethod
    def _ensure_path_closed(path):
        """Method to ensure that a path contains only closed sub-paths."""
        polygons = path.to_polygons()
        codes, vertices = [], []
        for p in polygons:
            vertices.extend([p[0], *p])
            codes.extend([
                mpath.Path.MOVETO, 
                *[mpath.Path.LINETO]*(len(p) - 1),
                mpath.Path.CLOSEPOLY])
                
        return mpath.Path(vertices, codes)

However I realized that even though the docs claim that no path simplification is performed, the resulting output suggests otherwise... (I did not look in the corresponding c-code)

Here's a gif showing the above to_polygons implementation (LEFT) and the current implementation (RIGHT):
(both use the exact same geometry as input)

The docs of to_polygons claim that

"If width and height are both non-zero then the lines will be simplified so that vertices outside of (0, 0), (width, height) will be clipped."

but the default (width=0, height=0) still seems to apply some kind of simplification (or maybe floating-point rounding erors?).

I'm not sure if this is a bug or if this is the intended behavior but in both cases it does not look like it's doing what we want it to do.

[greglucas]

It looks like the path is allowed to simplify still as it enters that method. If you put in path.should_simplify = False, then I think it might work as expected.

[raphaelquast]

Hey, indeed that did the job!
I think this behavior should definitely be mentioned somewhere in the to_polygons method as well...
There was no indication that an additional simplification of the path is triggered if should_simplify is True.

My final question now is if we should "reset" the should_simplify property to its initial value after conversion to avoid changing the properties of the path instance, e.g. something like this (or maybe with a contextmanager):

        initial_should_simplify = path.should_simplify
        path.should_simplify = False
        polygons = path.to_polygons(closed_only=False)
        path.should_simplify = initial_should_simplify

[greglucas]

My final question now is if we should "reset" the should_simplify property to its initial value after conversion to avoid changing the properties of the path instance, e.g. something like this (or maybe with a contextmanager):

👍 The context manager sounds like a good idea to me.

[raphaelquast]

@greglucas I now pushed a new implementation using to_polygons() and a contextmanager to temporarily disable path simplification.

[greglucas]

Thanks @raphaelquast, I like the use of to_polygons() 👍 Just some minor nits at this point.

[raphaelquast]

@greglucas
OK, I've updated the code to apply _ensure_path_closed already (and only) on ax.set_boundary() because in my opinion this is actually the proper way to do it.

• It completely avoids the hot-loop of checking for polygon closure during pan/zoom
• It is actually required to ensure that path.clip_to_bbox (used during pan/zoom) works as expected

---

However, this unfortunately causes a lot of other image tests to fail.
I'm not 100% sure why this has such broad implications, I guess it might be that a lot of the boundary-paths are actually not properly closed...

While checking the resulting changes, I believe that this has in fact a mostly positive impact on the exported images!

To clarify I've selected some of the most important changes I've found so far:

[click to show] some image comparison results

(LEFT: baseline, RIGHT: new result)

• With the new results it became visible that some of the baseline images actually have quite distorted text!
  (this is True for a lot of baseline images)

• The gridliner seems affected...
  • we lose +-180° labels for WebMercator (which is actually somewhat OK)

• but in other cases initially missing labels are now properly plotted (like the 0° label in the top right map)

Its quite unfortunate that this has such broad effects, but it seems like it's worth investigating what's going on here...
What do you think about that?

[greglucas]

We just keep going deeper here :(

It looks like the path is cleaned to remove duplicates with the to_polygons which must push some of the image checks just over the tolerances. I don't quite understand why that would affect any of the calculations though...

Original path input:

Path(array([[-180.,  -90.],
       [-180.,  -90.],
       [-180.,   90.],
       [ 180.,   90.],
       [ 180.,  -90.],
       [-180.,  -90.]]), array([ 1,  2,  2,  2,  2, 79], dtype=uint8))

After your new method:

Path(array([[-180.,  -90.],
       [-180.,   90.],
       [ 180.,   90.],
       [ 180.,  -90.],
       [-180.,  -90.]]), array([ 1,  2,  2,  2, 79], dtype=uint8))

Some of the original images do look better to me because they do a better job at clipping and not "leaking" the colors into the boundary. So maybe the best course of action is to just bump the tolerances ever so slightly up 0.02-0.03 for most of them it looks like?

[raphaelquast]

Yes I know... I really didn't expect that this final change would result in such a wide range of effects...

I did some more tests to get to the bottom of this:
It seems that path.clip_to_bbox does not return only closed polygons as I initially expected, so making sure that they are closed during pan/zoom seems still necessary...

If we run _ensure_path_closed both on spine._adjust_location and ax.set_boundary then all "leaking" is gone and
(locally on Windows) test-fails are only related to:

• Fonts in exported images are no longer distorted
• Some Grid-label differences
  • -180/180 are removed in WebMercator
  • Some initially missing labels are now properly displayed

On GitHub tests seem to succeed

[greglucas]

Looks like the final failing test is because this needs to be marked as a network test.

Otherwise I think this is good to go and will be a nice addition.

[raphaelquast]

Awesome! One final thought...
Should we apply the same treatment also to the _ViewClippedPathPatch?
(E.g. putting _ensure_path_closed outside the class and then using it for both the spine and the patch)

cartopy/lib/cartopy/mpl/geoaxes.py

Line 231 in 2d4cef2

def set_boundary(self, path, transform):

[greglucas]

Should we apply the same treatment also to the _ViewClippedPathPatch?
(E.g. putting _ensure_path_closed outside the class and then using it for both the spine and the patch)

That also seems reasonable to add this capability there. Do you want to do that here or in a follow-up PR? (I'm not sure where we'd want the _ensure_closed_poly to live, maybe in cartopy.mpl.patch as a private method there rather than on either of the classes?)

[raphaelquast]

Since at this stage it's just about agreeing on the right place for _ensure_path_closed, I'm fine with including it in this PR.

I definitely agree that _ensure_path_closed should be put outside the casses.
My suggestion would have been to just make it a private method in cartopy.mpl.geoaxes since both affected classes are defined there anyways.

However, If you think it's better to put it in cartopy.mpl.path I have no objections... just let me know what you prefer!

[greglucas]

I don't have a major preference either way. It just seemed like it is similar to the other path handling in that location, but we are only using it in this file so that is fine too. Either one seems reasonable.

[raphaelquast]

OK, all done!

In the end I decided to go with your suggestion... it seems like a better place for the method in case it is used somewhere else as well in the future.

[raphaelquast]

I guess the one failing test needs a re-run..
(fails with ERROR tests/mpl - PermissionError: [Errno 13] Permission denied: 'C:\\Users\\runneradmin\\.matplotlib\\fontlist-v390.json.matplotlib-lock')

[greglucas]

There is a branch protection rule on the Contributor License Agreement, can you sign that as well?
https://cla-assistant.io/SciTools/cartopy?pullRequest=2362

[raphaelquast]

Oh, I totally forgot that I need to re-sign this... done!

[greglucas]

Awesome, thank you @raphaelquast for the persistence and nice addition!

[raphaelquast]

Sure! Thanks for your valuable inputs!

[raphaelquast]

Hey, quick question :-)
Since this is already merged since quite a while... is there a chance of getting a release that includes this fix anytime soon?