Fix bug causing interior polygons to be ignored (clean)

[stephenworsley]

When path_to_geos read a path with the codes ...,1,2,79,... it would create a LineString which would be appended to collection. This would cause further path_codes to be ignored. This was causing a bug with contourf where occasionally these codes will be passed. This fix would cause these LineStrings to be added to other_result_geoms instead. I have not yet explored the possible consequences of passing LineStrings to other_result_geoms.

(This pull request is essentially the same as #1314 but with a cleaner commit history since it got muddled with my other branch at some point)

[QuLogic]

Can you post some sort of before/after images?

[stephenworsley]

@QuLogic here's an example.
Before the fix:

setting alpha to 0.5, we can see that there is a polygon which is being plot but overwritten.

After the fix:

[QuLogic]

Is Path([[1, 1], [1, 1], [1, 2]], [1, 2, 79]) specifically a path that doesn't work, or is it a simplification? What path is in the sample image above? Can you post code for that?

The problem with this specific path is that the 79 code means go-to-first-point, and [1, 2] is ignored. This means that part is in fact a single point. So if this path is specifically the problem, then I think we should look into improving the verts_same_as_first check instead.

[QuLogic]

For example, what if we did this?

     for path_verts, path_codes in zip(verts_split, codes_split):
         if len(path_verts) == 0:
             continue
 
+        if path_codes[-1] == Path.CLOSEPOLY:
+            path_verts[-1, :] = path_verts[0, :]
+          
         verts_same_as_first = np.all(path_verts[0, :] == path_verts[1:, :],
                                      axis=1)
         if all(verts_same_as_first):
             geom = sgeom.Point(path_verts[0, :])

I think this fixes your test (other than a type change, which is arguably more correct), but does it fix the above image?

[stephenworsley]

@QuLogic I've tried the above piece of code and it doesn't seem to fix the plot. I believe the path in the test is a simplification. I may not have entirely captured the nature of the path causing the bug in the plot (beyond the fact that it was length 3 and contained not all the same points). I'll dig back into the code and try and find what the exact path looked like.

[stephenworsley]

@QuLogic The exact values of path_verts are
[[193.75, -14.166664123535156], [193.75, -14.166664123535158], [193.75, -14.166664123535156]]
so a more accurate simplification for the tests would be
[[1, 1], [1, 2], [1, 1]]

[stephenworsley]

The following code, placed just before verts_same_as_first is created, seems to work.

if len(path_verts) == 3:
    path_verts[1:, :] = path_verts[0, :]

Would this also be a preferable solution?

[QuLogic]

I'm not sure about that, something like [[0, 0], [1, 0], [0, 0]] is still a line that should be drawn, isn't it? At least it does in Matplotlib.

Going back to your example, I think we should tweak the check for a Point so that it uses a tolerance instead of a straight equality. I've opened #1456. Let me know if that fixes it.

I do think we're handling LineString a bit weird still, but I'm not sure what the correct behaviour is.

[stephenworsley]

I can confirm that #1456 also fixes the plot, looks promising.

[QuLogic]

Since #1456 fixes your plot and is now merged, I'm going to block this. This PR may no longer be needed for you, but I also think some of it might still be applicable. I will need to think about it a bit more though.

[marqh]

Hi @QuLogic

please may i ask for a status update w.r.t. your comment:

Since #1456 fixes your plot and is now merged, I'm going to block this. This PR may no longer be needed for you, but I also think some of it might still be applicable. I will need to think about it a bit more though.

Do you still think it worthwhile keeping this open as a PR, or is it reasonable to close this now?

thank you
marqh