Fix for 'linestring being readded' bug

[djkirkham]

When attaching lines to the boundary, the points where the lines meet the boundary, plus the boundary points, are put into a list and sorted by their distance along the boundary. If two lines happen to have an end-point at the same location then there is a risk that the end-points of the two lines will be interleaved, which causes the algorithm to fail later on with the 'linestring being re-added' error. This PR adds extra elements to the key used for the sort to prevent this happening.

[dopplershift]

Can you add a test case that demonstrates the problem?

[djkirkham]

I've changed the fix to a cleaner implementation. The basic idea now is to only consider 'first' points when looking for new points to add to the current linestring. The justification for this is that, assuming the exterior linestring of a polygon has the opposite orientation (clockwise or counter-clockwise) to all interior linestrings, then when traversing the boundary looking for the end point of the next line to add to the current linestring, we should only encounter 'first' points; if we encounter a 'last' point then it can only be that it is at the same location as a 'first' point. As to whether the above assumption is justified, the algorithm already implicitly assumes this: when beginning to process a new linestring in the first iteration of the loop at line 455 it chooses to process the line from 'first' to 'last'. This is only valid if the above assumption is true.

It turns out that the root cause of the bug doesn't always result in an error. It can simply result in the polygon encompassing the whole map. I've added two tests which demonstrates the two cases.

The test test_same_points_on_boundary_2 demonstrates the original issue (the error). When running the content of the test with debug = True at line 340 in crs.py the following output is produced running with the current code:

Edge things
    _BoundaryPoint(0.0, True, POINT (-180 -90))
    _BoundaryPoint(360.0, True, POINT (180 -90))
    _BoundaryPoint(430.0, False, (1, 'last', (179.99996185302734, -20.0)))
    _BoundaryPoint(430.00000953674316, False, (5, 'first', (179.9999904632568, -19.999990463256836)))
    _BoundaryPoint(440.0, True, POINT (180 -10))
    _BoundaryPoint(449.99999046325684, False, (5, 'last', (179.9999904632568, -9.5367431640625e-06)))
    _BoundaryPoint(450.00000953674316, False, (2, 'first', (179.9999904632568, 9.5367431640625e-06)))
    _BoundaryPoint(460.0, True, POINT (180 10))
    _BoundaryPoint(469.99999046325684, False, (2, 'last', (179.9999904632568, 19.999990463256836)))
    _BoundaryPoint(470.0, False, (1, 'first', (179.99996185302734, 20.0)))
    _BoundaryPoint(540.0, True, POINT (180 90))
    _BoundaryPoint(900.0, True, POINT (-180 90))
    _BoundaryPoint(970.0, False, (0, 'last', (-180.0, 20.0)))
    _BoundaryPoint(970.0, False, (3, 'first', (-180.0, 20.0)))
    _BoundaryPoint(980.0, True, POINT (-180 10))
    _BoundaryPoint(990.0, False, (3, 'last', (-180.0, 0.0)))
    _BoundaryPoint(990.0, False, (4, 'first', (-180.0, 0.0)))
    _BoundaryPoint(1010.0, False, (0, 'first', (-179.99999999992724, -20.0)))
    _BoundaryPoint(1010.0, False, (4, 'last', (-180.0, -20.0)))
+
Processing: 0, LINESTRING (-179.9999999999272 -20, -160 -20, -160 20, -180 20)
   d_last: 970.0
   next_thing: _BoundaryPoint(980.0, True, POINT (-180 10))
   adding boundary point
   d_last: 980.0
   next_thing: _BoundaryPoint(990.0, False, (3, 'last', (-180.0, 0.0)))
   adding line
   d_last: 970.0
   next_thing: _BoundaryPoint(990.0, False, (4, 'first', (-180.0, 0.0)))
   adding line
   d_last: 1010.0
   next_thing: _BoundaryPoint(0.0, True, POINT (-180 -90))
   adding boundary point
   d_last: 0.0
   next_thing: _BoundaryPoint(360.0, True, POINT (180 -90))
   adding boundary point
   d_last: 360.0
   next_thing: _BoundaryPoint(430.0, False, (1, 'last', (179.99996185302734, -20.0)))
   adding line
   d_last: 470.0
   next_thing: _BoundaryPoint(540.0, True, POINT (180 90))
   adding boundary point
   d_last: 540.0
   next_thing: _BoundaryPoint(900.0, True, POINT (-180 90))
   adding boundary point
   d_last: 900.0
   next_thing: _BoundaryPoint(970.0, False, (3, 'first', (-180.0, 20.0)))
   adding line
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File ".../lib/cartopy/crs.py", line 176, in project_geometry
    return getattr(self, method_name)(geometry, src_crs)
  File ".../lib/cartopy/crs.py", line 327, in _project_polygon
    rings.extend(self._attach_lines_to_boundary(multi_lines, is_ccw))
  File ".../lib/cartopy/crs.py", line 509, in _attach_lines_to_boundary
    raise RuntimeError('Unidentified problem with '
RuntimeError: Unidentified problem with geometry, linestring being re-added. Please raise an issue.

Notice that the end points of linestrings enumerated 0 and 4 are interleaved. Because of this interleaving, when processing linestring 0, the algorithm 'jumps over' point (0, 'first') via linestring 4. It then goes all around the boundary and re-encounters linestring 3, which results in the error.

[pelson]

Awesome! Thanks @djkirkham.

[pelson]

P.S. I tested this with the code @djkirkham provided:

import cartopy.crs as ccrs
import shapely.geometry as sgeom
import matplotlib.pyplot as plt

source = ccrs.PlateCarree()
target = ccrs.PlateCarree(central_longitude=180)

geom = sgeom.Polygon([(-20, -20), (20, -20), (20, 20), (-20, 20)],
                     [[(-10, 0), (0, 20), (10, 0), (0, -20)]])

Before: Error (linestring being re-added)
After:

[pelson]

I do wonder whether this should be != 'last'.

[pp-mo]

Actually, I wasn't quite done with this...
Apart from a desire for better testing, I had a suspicion that the whole of crs.py line 397 onward is now obsolete,
i.e. not edge_thing.kind and not prev_thing.kind and edge_thing.data[0] == prev_thing.data[0] can no longer happen at all...
To be continued...

[pelson]

Actually, I wasn't quite done with this...

Please feel free to use the reviewing / assignment functionality.

I actually believe the testing is sufficient, though you may be correct about some of this making subsequent code obsolete. Happy to review a PR from yourself / @djkirkham to clear that bit up.

[pp-mo]

some of this making subsequent code obsolete

No its okay, I checked it through again and I was wrong !