fix: repair invalid AOI geometry before using it to trim the task grid - #7321
Open
dulcetberg wants to merge 1 commit into
Open
fix: repair invalid AOI geometry before using it to trim the task grid#7321dulcetberg wants to merge 1 commit into
dulcetberg wants to merge 1 commit into
Conversation
Related to hotosm#6604 (not a confirmed full reproduction - see below). GridService.merge_to_multi_polygon() validates the merged AOI using geojson.MultiPolygon.is_valid, which only checks structure (ring closure etc), not real topology. A self-intersecting AOI (e.g. from a hand-drawn or imported boundary) can be structurally well-formed while still being topologically invalid, so it passes this check unrepaired. That invalid AOI then gets used directly in per-tile intersection() calls in trim_grid_to_aoi(). Intersecting with an invalid input geometry is undefined behavior in GEOS/Shapely and can produce corrupted (self-intersecting) output even from a clean square input tile. Fix: after building multi_polygon (a real Shapely geometry), check its actual topological validity and repair with buffer(0) if needed, before converting to geojson and running the existing checks. Also handles the case where buffer(0) collapses a MultiPolygon down to a plain Polygon (same thing _dissolve already guards against for unary_union's output a few lines below) - forces it back to MultiPolygon so the existing type check doesn't reject a valid repair. Investigation: pulled project 5349's actual AOI and the 7 flagged task geometries from the public production API (they're publicly accessible). Confirmed all 7 tasks are genuinely invalid (self-intersecting rings, matching the reported "weird lines" that don't change task area), and confirmed the project's stored AOI is itself invalid via Shapely's real validity check, while geojson.MultiPolygon.is_valid on the same geometry reports valid - exactly the gap this fix closes. Verified buffer(0) repairs the real AOI losslessly (identical area and bounds, valid=True after). Could not reproduce project 5349's exact corrupted task squares by constructing guessed grid tiles against the real AOI - I don't have access to the actual original tile geometries the grid-generation process used for that project, so I can't fully confirm this mechanism is what produced that project's specific corruption. This PR is a genuine, independently-justified correctness fix (using invalid geometry in intersection operations is unsafe regardless), not a confirmed fix for hotosm#6604 specifically. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Brian Bergstrom <dulcetberg@gmail.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Summary
Related to #6604 — see the investigation notes below on why I'm not claiming this fully reproduces/fixes it.
GridService.merge_to_multi_polygon()validates the merged AOI usinggeojson.MultiPolygon.is_valid, which only checks structure (ring closure etc), not real topology. A self-intersecting AOI (e.g. from a hand-drawn or imported boundary) can be structurally well-formed while still being topologically invalid, so it passes this check unrepaired.That invalid AOI then gets used directly in per-tile
intersection()calls intrim_grid_to_aoi(). Intersecting with an invalid input geometry is undefined behavior in GEOS/Shapely and can produce corrupted (self-intersecting) output even from a clean square input tile.Fix: after building
multi_polygon(a real Shapely geometry), check its actual topological validity and repair withbuffer(0)if needed, before converting to geojson and running the existing checks. Also handles the case wherebuffer(0)collapses aMultiPolygondown to a plainPolygon(the same thing_dissolvealready guards against forunary_union's output a few lines below) — forces it back toMultiPolygonso the existing type check doesn't reject a valid repair.Investigation
Pulled project 5349's actual AOI and the 7 flagged task geometries from the public production API (they're publicly accessible). Confirmed:
geojson.MultiPolygon.is_validon the same geometry reports valid — exactly the gap this fix closes.buffer(0)repairs the real AOI losslessly (identical area and bounds,valid=Trueafter).What I couldn't confirm: I wasn't able to reproduce project 5349's exact corrupted task squares by constructing guessed grid tiles against the real AOI — I don't have access to the actual original tile geometries the grid-generation process used for that project. So I can't fully confirm this specific mechanism is what produced that project's corruption. This PR is a genuine, independently-justified correctness fix (using invalid geometry in intersection operations is unsafe regardless of this specific incident), not a confirmed fix for #6604.
Test plan
geojson.MultiPolygon.is_validreportsTruedespite Shapely's real check reportingFalse; after the fix, the repaired geometry is genuinely valid, keeps itsMultiPolygontype, and preserves the original area/bounds exactly.if not multi_polygon.is_valid, so any already-valid AOI (what the existing tests use) takes the identical code path as before — no behavior change for the valid-input case.🤖 Generated with Claude Code