Repository navigation
fix: don't drop multipolygon parts whose seed lands in an earlier part - #30
Merged
Merged
Conversation
polygons_to_geohashes shares `accepted_geohashes` across the parts of a multipolygon, but the early `continue` skipped neighbour expansion as well as re-testing. A part whose seed cell had already been accepted while walking an earlier part therefore terminated on its own seed and contributed nothing to the output. With A = rect(-73.60, 45.50, -73.55, 45.55) and B = rect(-73.590, 45.520, -73.520, 45.525), B's centroid falls inside A, and at precision 7 the function returned A's 1369 cells where the union of the parts is 1479 — 110 cells (7%) silently missing, and 66 of 1291 with fully_contained_only. Track a per-polygon `visited` set that gates the geometry test while `accepted` stays global, and always expand from any cell that intersects. Each part now gets a full walk regardless of what earlier parts covered, which also makes the separate `rejected` set redundant. Two smaller fixes in the same loop: - `neighbor.to_string()` cloned an already-owned String, costing eight wasted allocations per visited cell. - `polygon.unsigned_area()` was recomputed inside the loop for every candidate cell; it is O(vertices) and constant, so hoist it. Worth ~8% on verdun p9 with fully_contained_only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
Author
|
This change is part of the following stack:
Change managed by git-spice. |
This was referenced Aug 25, 2026
Merged
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes an under-coverage bug in polygons_to_geohashes when processing multiple polygon parts (e.g., MultiPolygon parts) where a later part’s BFS would previously terminate early if its seed cell had already been accepted while walking an earlier part. It also removes some avoidable per-cell overhead and adds regression tests to prevent reintroducing the issue.
Changes:
- Introduces a per-polygon
visited_geohashesset so each polygon part gets a full BFS walk regardless of globally accepted cells. - Removes the per-polygon
rejected_geohashestracking and avoids unnecessaryStringallocations during neighbor expansion. - Hoists
polygon.unsigned_area()out of the inner loop and adds two regression tests (union property + order-independence).
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Problem
polygons_to_geohashessharesaccepted_geohashesacross the parts of a MultiPolygon, but the earlycontinueskipped neighbour expansion, not just re-testing. A part whose seed cell had already been accepted while walking an earlier part terminated on its own seed and contributed nothing.With
A = rect(-73.60, 45.50, -73.55, 45.55)andB = rect(-73.590, 45.520, -73.520, 45.525)— B's centroid falls inside A — precision 7 returned A's 1369 cells where the union of the parts is 1479. 110 cells (7%) silently missing, and 66 of 1291 withfully_contained_only. No error raised.Fix
Track a per-polygon
visitedset that gates the geometry test whileacceptedstays global, and always expand from any cell that intersects. Each part now gets a full walk regardless of what earlier parts covered, which also makes the separaterejectedset redundant.Two papercuts in the same loop:
neighbor.to_string()cloned an already-ownedString— 8 wasted allocations per visited cell.polygon.unsigned_area()was recomputed per candidate cell; it is O(vertices) and constant. Hoisting it is worth ~8% on verdun p9 withfully_contained_only.Tests
Two regression tests, both failing on
main: the union property, and order-independence of the parts.