Validation::is_valid #1279

michaelkirk · 2024-12-09T23:33:55Z

I agree to follow the project's code of conduct.
I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

Much of the original code and test cases are courtesy of https://github.com/mthh/geo-validity-check (thanks @mthh!).

I also enabled the JTS IsValid test cases from our existing JTS test suite.

My approach:

First, I just copy/pasted the existing implementation from https://github.com/mthh/geo-validity-check in: 92d8bfc

And then wired it into our crate: 5dd1909

Most significantly, I did a lot of refactoring in pursuit of deduplicating code in b89e715, so any bugs are my own.

Note to merger: please don't squash these commits, they're somewhat meaningful.

geo/Cargo.toml

michaelkirk · 2024-12-10T00:45:21Z

geo/src/algorithm/relate/geomgraph/edge_end_bundle_star.rs

+                    #[cfg(debug_assertions)]
+                    if right_position != current_position {
+                        use crate::algorithm::Validation;
+                        if geometry_graph.geometry().is_valid() {


This should address #1268

right_position != current_position should not happen normally, and this debug_assertion was originally intended to find bugs in geo code. However, it's also been shown to happen when given invalid geometry input.

We can now distinguish between a hypothetical bug in geo code from a bug in the caller's code, and just log in that latter case.

michaelkirk · 2024-12-10T04:03:52Z

@mthh - since this is largely based on your implementation, and you've done some work with validation, would you mind reviewing this?

frewsxcv

Exciting! Can't wait for this to land

Definitely not done reviewing, just did a first pass and added a few comments

geo/CHANGES.md

geo/src/algorithm/validation/mod.rs

frewsxcv

Another pass

geo/src/algorithm/validation/geometry.rs

frewsxcv · 2024-12-11T01:53:00Z

geo/src/algorithm/validation/mod.rs

+    /// The closure `handle_validation_error` is called for each validation error.
+    fn visit_validation<T>(
+        &self,
+        handle_validation_error: Box<dyn FnMut(Self::Error) -> Result<(), T> + '_>,


Presumably something like this wasn't possible?

handle_validation_error: FnMut(Self::Error) -> Result<(), T>,

Because of the potentially infinite recursion of GeometryCollection, we cannot use a true generic impl FnMut - it must be boxed.

Is that what you were asking about?

Here's the error message when you switch to: handle_validation_error: impl FnMut(Self::Error) -> Result<(), T>,

error: reached the recursion limit while instantiating `validation::geometry_collection::<impl Validation for geo::GeometryCollection>::visit_validation::<geo::validation::InvalidGeometry, Box<{closure@validation::geometry::<impl Validation for geo::Geometry>::visit_validation<..., ...>::{closure#7}}>>` --> /Users/mkirk/src/georust/geo/geo/src/algorithm/validation/geometry.rs:91:48 | 91 | Geometry::GeometryCollection(g) => g.visit_validation(Box::new(|err| { | ________________________________________________^ 92 | | handle_validation_error(InvalidGeometry::InvalidGeometryCollection(err)) 93 | | }))?, | |_______________^ | note: `validation::geometry_collection::<impl Validation for geo::GeometryCollection<F>>::visit_validation` defined here --> /Users/mkirk/src/georust/geo/geo/src/algorithm/validation/geometry_collection.rs:28:5 | 28 | / fn visit_validation<T>( 29 | | &self, 30 | | mut handle_validation_error: impl FnMut(Self::Error) -> Result<(), T>, 31 | | ) -> Result<(), T> { | |______________________^ = note: the full type name has been written to '/Users/mkirk/src/georust/geo/target/debug/deps/jts_test_runner-201a42bc420bd6fb.long-type.txt'

I was thinking something like handle_validation_error: impl FnMut(Box<Self::Error>) -> Result<(), T>,

I don't think that addresses the potentially infinite recursion of the closure - without boxing the closure, the compiler must build a type which can store the state of a closure which validates a GeometryCollection inside a GeometryCollection inside a GeometryCollection inside a...

I confess I'm not that confident in my analysis, but I did have a go at your suggestion here, but ultimately hit the same compiler error:

Reached the recursion limit while instantiating validation::geometry_collection::<impl Validation for geo::GeometryCollection>::visit_validation::<geo::validation::InvalidGeometry, {closure@validation::geometry::<impl Validation for geo::Geometry>::visit_validation<geo::validation::InvalidGeometry, ...>::{closure#7}}>

geo/src/algorithm/validation/mod.rs

frewsxcv

I replied with one more comment but this looks good to me! Thanks for putting the effort into making this happen

frewsxcv · 2024-12-17T21:16:53Z

geo/src/algorithm/validation/mod.rs

+    /// The closure `handle_validation_error` is called for each validation error.
+    fn visit_validation<T>(
+        &self,
+        handle_validation_error: Box<dyn FnMut(Self::Error) -> Result<(), T> + '_>,


I was thinking something like handle_validation_error: impl FnMut(Box<Self::Error>) -> Result<(), T>,

michaelkirk · 2024-12-18T00:46:28Z

Thanks for the review @frewsxcv!

I'll give it another day before merging in case anyone else wants to look.

mthh · 2024-12-18T06:41:35Z

@michaelkirk thank you for inviting me to review it, unfortunately I won't have the time to do so.
Anyway I'm glad to see the Validation trait land in geo, thanks for the effort :)

michaelkirk · 2024-12-19T15:08:55Z

See #1283 for details on the unrelated CI failure.

325ec00e5a1207308eecfb1d8fb5456cd5d5d121 from https://github.com/michaelkirk/geo-validity-check

Note: some tests are failing

failed TestValid2.xml case "Test 740" with error: expected true, actual: false failed TestValid2.xml case "Test 749" with error: expected true, actual: false failed TestValid2.xml case "Test 752" with error: expected true, actual: false failed TestValid2.xml case "Test 753" with error: expected true, actual: false failed TestValid2.xml case "Test 754" with error: expected true, actual: false Note: this commit is empty, just a commit message.

We follow JTS/GEOS convention here.

Previously the `is_valid` vs `explain_invalidity` methods were mostly a cut/paste job. `explain_invalidity` gathered all the errors, while `is_valid` short-circuited to return early as soon as it new a geometry was invalid. Now both methods share a code path, in the new `visit_validation` method. And there's another new method: `check_validation` which, because it returns a result can be used nicely in error handling: ``` polygon.check_validation()?; ``` This also leverages generics (associated types) to return specific error cases per geometry, which replaces some runtime `unreachable` checks with compile time type checks.

Co-authored-by: Corey Farwell <coreyf@rwell.org>

… feature anyway

Co-authored-by: Corey Farwell <coreyf@rwell.org>

michaelkirk force-pushed the mkirk/mthh_is_valid_2 branch 3 times, most recently from 69757d9 to 7b5225f Compare December 10, 2024 00:22

michaelkirk mentioned this pull request Dec 10, 2024

install geos georust/docker-images#37

Closed

michaelkirk force-pushed the mkirk/mthh_is_valid_2 branch 2 times, most recently from 7f140cc to ed0e43a Compare December 10, 2024 03:55

michaelkirk commented Dec 10, 2024

View reviewed changes

michaelkirk marked this pull request as ready for review December 10, 2024 04:02

michaelkirk requested a review from mthh December 10, 2024 04:02

michaelkirk requested review from urschrei and frewsxcv December 10, 2024 04:07

frewsxcv reviewed Dec 10, 2024

View reviewed changes

geo/CHANGES.md Outdated Show resolved Hide resolved

geo/src/algorithm/validation/mod.rs Show resolved Hide resolved

geo/src/algorithm/validation/mod.rs Outdated Show resolved Hide resolved

frewsxcv reviewed Dec 11, 2024

View reviewed changes

frewsxcv approved these changes Dec 17, 2024

View reviewed changes

michaelkirk added this pull request to the merge queue Dec 18, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 18, 2024

michaelkirk added 10 commits December 19, 2024 14:30

extract test runner fn

4a5dde6

add validation tests from JTS test suite

f4a7ec5

cp -r ../geo-validity-check/src/* geo/src/algorithm/validation/

45a3eb3

325ec00e5a1207308eecfb1d8fb5456cd5d5d121 from https://github.com/michaelkirk/geo-validity-check

integrate geo-validity-check into geo

8af9864

Note: some tests are failing

NBD: naming nits

bf14df5

All JTS tests passing: EMPTY geometries are considered valid.

107ed5f

We follow JTS/GEOS convention here.

Warn rather than debug_assert with invalid geometries

641853b

DRY up GEOS integration

1559ded

michaelkirk and others added 8 commits December 19, 2024 14:30

changelog entry

fdbd4b2

use latest ci images

361ebc1

Update geo/CHANGES.md

3651525

Co-authored-by: Corey Farwell <coreyf@rwell.org>

use std::error::Error

c58d61a

add top level doc

3d30ba7

remove geos tests over concerns about licensing. it wasn't a critical…

fd7c4b5

… feature anyway

Update geo/src/algorithm/validation/geometry.rs

e35499b

Co-authored-by: Corey Farwell <coreyf@rwell.org>

make inner value pub for index types

2b4ded7

michaelkirk force-pushed the mkirk/mthh_is_valid_2 branch from b4d6493 to 2b4ded7 Compare December 19, 2024 20:34

michaelkirk enabled auto-merge December 19, 2024 20:36

michaelkirk added this pull request to the merge queue Dec 19, 2024

Merged via the queue into main with commit 785c1d8 Dec 19, 2024
18 checks passed

michaelkirk deleted the mkirk/mthh_is_valid_2 branch December 19, 2024 20:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validation::is_valid #1279

Validation::is_valid #1279

michaelkirk commented Dec 9, 2024 •

edited

Loading

michaelkirk Dec 10, 2024

michaelkirk commented Dec 10, 2024

frewsxcv left a comment

frewsxcv left a comment

frewsxcv Dec 11, 2024

michaelkirk Dec 11, 2024

michaelkirk Dec 11, 2024

frewsxcv Dec 17, 2024

michaelkirk Dec 18, 2024

frewsxcv left a comment

frewsxcv Dec 17, 2024

michaelkirk commented Dec 18, 2024

mthh commented Dec 18, 2024

michaelkirk commented Dec 19, 2024

Validation::is_valid #1279

Validation::is_valid #1279

Conversation

michaelkirk commented Dec 9, 2024 • edited Loading

My approach:

michaelkirk Dec 10, 2024

Choose a reason for hiding this comment

michaelkirk commented Dec 10, 2024

frewsxcv left a comment

Choose a reason for hiding this comment

frewsxcv left a comment

Choose a reason for hiding this comment

frewsxcv Dec 11, 2024

Choose a reason for hiding this comment

michaelkirk Dec 11, 2024

Choose a reason for hiding this comment

michaelkirk Dec 11, 2024

Choose a reason for hiding this comment

frewsxcv Dec 17, 2024

Choose a reason for hiding this comment

michaelkirk Dec 18, 2024

Choose a reason for hiding this comment

frewsxcv left a comment

Choose a reason for hiding this comment

frewsxcv Dec 17, 2024

Choose a reason for hiding this comment

michaelkirk commented Dec 18, 2024

mthh commented Dec 18, 2024

michaelkirk commented Dec 19, 2024

michaelkirk commented Dec 9, 2024 •

edited

Loading