-
-
Notifications
You must be signed in to change notification settings - Fork 261
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Interior rings incorrectly converted to GeoJSON #202
Comments
@QuLogic thanks for raising this issue, and apologies for the late response. You're correct about how pyshp parses the shapefile rings. As you say, since the shapefile spec doesn't specify any particular order of the rings or how they are grouped, the only way to know which holes belong to which exteriors is to check each for containment. However, the thinking was that doing polygon in polygon tests for each would be computationally expensive and complex, and I think I read somewhere that "in practice" it's ok to assume that most polygon shapefiles are written as the exterior ring first, followed by each of its interior holes (until the next polygon exterior in the case of MultiPolygon). I've used and rendered geojson representations of complex polygon shapefiles based on this implementation for years and haven't encountered any rendering issues related to this before. However, it's clear that the shapefile you used did not follow this (and might be more widespread than I have assumed), and at any rate this ordering cannot be guaranteed as per the shapefile spec. So I agree that doing a more proper hole-exterior containment testing would be the most robust way forward. I'd be willing to implement this, as long as there is a feasible and efficient way to do this in pure python. Do you recall specifically how Cartopy used to do hole-in-polygon testing? I wonder if it would be sufficient to take the bbox centroid of the hole and do a simple point-in-polygon test (if the entire hole is to be contained in the exterior, then surely the hole-centroid must as well)? |
I believe this GDAL ticket was what I based on in assuming ring ordering, and was the same issue they grappled with. It seems that in the end they went for proper polygon testing, so even more reason to go in that direction. |
I could point you at Cartopy's old code, but it's LGPL. What it does is create two lists of Shapely |
Hey @QuLogic, so I believe I fixed the issue, ended up implementing a series of pure-Python fast containment tests: starting with a simple bbox hole-in-exterior overlap test, and then for holes that couldn't be clearly linked to a single exterior do a fast sample-point hole-in-exterior test. Based on your test code, things seem to be working as expected now:
You can check out the details of the new code starting at line 321. However, if a hole is found to be contained by multiple exteriors, that means there's a hole nested inside an exterior nested inside a hole etc (eg a small lake on an island in a large lake in a country), and additional testing would have to be done to determine the most immediate parent exterior. For now I'm just throwing a NotImplementedError. Do you remember if you handled such scenarios in the previous Cartopy code / do you think this would be a priority? |
I don't believe the old code handled anything like that. Holes were only put in a single exterior, IIRC. |
I just came across a shapefile which threw an error due to nested exteriors/holes, so quickly added handling of this, wasn't too complicated. Also seems exceedingly rare, but a possibility nonetheless. |
This is now included in v2.1.1 on PyPI. |
In Cartopy, we recently switch from manual conversion of Shapefile-to-Shapely to using
shapely.geometry.shape
, which uses pyshp's implementation of__geo_interface__
. On the 10m scale Natural Earth Land feature, this produces bogus results SciTools/cartopy#1578. This is because the__geo_interface__
implementation for polygons returns invalid polygons.If you load
ne_10m_land
, and look at the first shape, you get a ring of 98 parts.pyshp takes interior rings and puts them in the preceding exterior ring:
pyshp/shapefile.py
Lines 244 to 250 in 71231dd
so here, part 15 is used as an interior ring in part 14:
However, if you check more carefully, that ring is not contained in part 14, but in part 4:
The original Cartopy code, traversed all exterior rings and put the interior ring in the one that contains it (i.e., part 15 is put in part 4). It seems that this traversal must be done in pyshp as well, or invalid geometries are produced.
If the interior ring is moved to the right place, the geometry is valid again:
The text was updated successfully, but these errors were encountered: