Add ImplHelper::ValidOp Module to Provide Uniform Validity Checks #271
Conversation
…idity checking of geometries. The ValidOp module provides methods based on simple geometric predicates to determine validity for a geometry and can be used as a fallback or overridden. The module can be overriden fully (in the case of Cartesian polygons) or just by implementing #invalid_reason (FFI and CAPI factories). Removes #validate_geometry from all classes and replaces them with #prepare_geometry. Any required validations were moved to the constructors. Adds the ValidityTest module to test/common to ensure that all classes of geometries validate in the same way.
There was a problem hiding this comment.
I think the way ValidOp is included is fine. As discussed in one of my comments, we can just override leaf methods (i.e. the one that does the final check) to have something more accurate per implementation when needed.
If some method is not a leaf and we need to override a part of it, we still can change the parent method, to make sure we always have a minimal overriding part.
Also, that has been a lot of work you've done on that validity stuff, thank you very much! It is a pleasure reading all of it 🙂
| return ImplHelper::TopologyErrors::UNCLOSED_RING unless exterior_ring.is_closed? | ||
| return ImplHelper::TopologyErrors::UNCLOSED_RING unless interior_rings.all?(&:is_closed?) |
There was a problem hiding this comment.
When i see the is_closed usage there, I feel like we'll have some trouble rebase with the master branch if we wait too much
There was a problem hiding this comment.
Yeah once it gets merged I can go back and change it.
| # TODO: should we replace with graph.incident_edges.length == segments.length | ||
| # since graph isn't used elsewhere yet it might be more overhead. |
There was a problem hiding this comment.
Not 100% sure I get your point there ^^
There was a problem hiding this comment.
The same simplicity check (self-intersection) can be performed by creating the GeometryGraph of the LineString and doing the described comparison. But doing the check with graph will be slower since it has to perform an intersection check and then link all of the nodes whereas the current implementation only does an intersection check.
And since graph is not used elsewhere in any LineString method I figured the additional overhead is not necessary, but I could change it if we think it's better to do so.
ValidOp is to be included in feature classes and has the invalid_reason and valid? methods, as well as feature type specific methods that walk through the validity flow for each geometry type. ValidOpHelpers is a collection of functions where an object is passed to them. These perform the specific checks and return the invalid reason if applicable. Also added overrides for both modules for cartesian functions.
…nd fix tests, ValidOp so the spherical factory passes validity checks
I'll definitely do that bit. In fact I think we're ready to merge, since what is left is either optimisation, or spherical support. By merging we'll be able to split that job in many and have me and contributors help you :) |
|
|
||
| private | ||
|
|
||
| # TODO: replace with better algorithm |
There was a problem hiding this comment.
This should be translated into an issue I think :)
Summary
Implements
ImplHelper::ValidOpwhich provides implementations of thevalid?andinvalid_reasonmethods based on simple geometric predicates.Change summary:
ImplHelper::ValidOpValidOpfor cartesian polygons and use theGeometryGraphto implement the checks.test/common/validity_testto ensure uniform validity across all implementation types.validate_geometrywithprepare_geometryand move any relevant validations to the constructor.LinearRinghaving enough points that GEOS will raise for so we have to also.prepare_geometrywill do the handy things thatvalidate_geometrydid like closing rings and ensuring points are in a projection area.Other Information
Tests are currently failing because of the spherical factory
There are still some open issues that need to be figured out before we can merge. The most pressing of which is the spherical implementation.
TODOS:
ValidOp. It seems a little clunky in cartesian polygons right now because a lot of the code is copied.ValidOpmethods with graph implementations. Once we've determined that the overriding structure makes sense I'll add more.uses_lenient_multipolygon_assertionsanduses_lenient_assertions(this may be part of @BuonOmo's validity PR that provides safe and unsafe versions of methods).graphand re-implement some predicates with it (could be done later).