Allow polygon shape to consist of disjoint parts - #14376
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new union-based parsing can produce polygons with holes, but the current vertex serialization/re-hydration cannot represent holes, which can lead to incorrect contains() results.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates PolygonShapeConfig to support boundary files containing multiple polygons (treated as the union of all polygons), enabling seismic observation filtering against disjoint polygon parts.
Changes:
- Change
PolygonShapeConfig.verticesfrom a single ring to a list of polygon rings (multipolygon representation). - Update polygon parsing to build a union geometry from multiple polygons and preserve disjoint parts while merging overlaps.
- Expand unit test coverage for disjoint, overlapping, and normalization/simplification behavior for multipolygons, and update seismic QC tests accordingly.
File summaries
| File | Description |
|---|---|
src/ert/config/_shapes.py |
Switch polygon handling to multipolygons; union multiple polygons from file and update containment to work across all parts. |
tests/ert/unit_tests/config/test_shapes.py |
Update expectations for nested vertex structure and add tests for disjoint/overlapping/normalized multipolygons. |
tests/ert/unit_tests/config/test_observation_quality_control.py |
Verify seismic QC keeps observations inside any of multiple boundary polygons. |
tests/ert/unit_tests/config/test_observation_declaration.py |
Update boundary parsing assertions to the new nested vertex representation. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #14376 +/- ##
==========================================
- Coverage 91.06% 91.05% -0.01%
==========================================
Files 495 495
Lines 36139 36146 +7
==========================================
+ Hits 32910 32913 +3
- Misses 3229 3233 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Per user's requirement polygon file may contain multiple disjoint polygons. For seismic, we need keep points that are inside one or more polygons. Polygon shape is for now used only in seismic and seismic is unreleased yet, so we are free to change it without considerations.
037798a to
190e35c
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The current multipolygon cleaning/parsing path can raise unintended runtime exceptions (and produce nondeterministic equality) due to insufficient geometry-type validation and use of assertions for config validation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
src/ert/config/_shapes.py:103
- Using
assert isinstance(..., MultiPolygon)for WKT parsing can raise anAssertionErrorfor user-supplied/serialized shapes (e.g. ifwktis aPOLYGON). SincePolygonShapeConfigis a config model, this should fail with a regular exception (or acceptPOLYGONby wrapping it) rather than an assertion that can be stripped with-O.
def _polygon(self) -> shapely.MultiPolygon:
multipolygon = shapely.from_wkt(self.wkt)
assert isinstance(multipolygon, shapely.MultiPolygon)
shapely.prepare(multipolygon)
return multipolygon
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
| cleaned_polygons = [ | ||
| cast( | ||
| shapely.Polygon, | ||
| geom.simplify(tolerance=cls.TOLERANCE).normalize(), | ||
| ) | ||
| for geom in multipolygon.geoms | ||
| ] | ||
| multipolygon = shapely.MultiPolygon(cleaned_polygons) | ||
|
|
| expected = "MULTIPOLYGON Z (((0 0 0, 0 1 0, 1 1 0, 1 0 0, 0 0 0)))" | ||
| assert boundary.wkt == expected |
Issue
Resolves #14297
Approach
Per user's requirement polygon file may contain multiple disjoint polygons. For seismic, we need keep points that are inside one or more polygons.
Polygon shape is for now used only in seismic and seismic is unreleased yet, so we are free to change it.
git rebase -i main --exec 'just rapid-tests')When applicable
merge screenshot-PR in ert-testdata before merging this PR.