Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Standardize Validation #228

Merged
merged 6 commits into from Nov 11, 2020
Merged

Standardize Validation #228

merged 6 commits into from Nov 11, 2020

Conversation

keithdoggett
Copy link
Member

@keithdoggett keithdoggett commented Oct 30, 2020

Summary

PR to fix #218 where is_simple? returns inconsistent results across libgeos versions.

Other Information

I personally tried using geos versions 3.5.2, 3.6.4, 3.7.3, and 3.8.1 across multiple Rubies and did not have a failing test case. I'm using Ubuntu Bionic so that may have something to do with it, but this is inconsistent with what is described in #218 .

I also looked at the libgeos docs and there is a note in there that suggests GeosIsSimple will always return true for polygons and that GeosIsValid would be the appropriate function to check for self-intersections. This is also described here https://postgis.net/docs/using_postgis_dbmanagement.html#OGC_Validity where it says "By definition, a POLYGON is always simple."

coffeejunk and others added 4 commits October 18, 2020 19:04
- Add builds for libgeos 3.4, 3.5, 3.6, 3.7, 3.8 (via ubuntu versions)
- Add osx build (with latest geos installed via homebrew)
- Add ruby 2.7
- Use jruby 9.2.13.0
Broaden travis ci build matrix
…or. Removed self-intersecting is_simple? test on CAPIPolygons because this will be version dependent.
@keithdoggett
Copy link
Member Author

After trying again, I was able to get tests to fail locally by switching libgeos versions. The issue is that in older versions of libgeos (<3.8), only LineStrings, MultiLineStrings and MulitPoints had a simplicity check while everything else returned true by default. This was modified by this commit and released in 3.8.0.

The simplicity check that the Geographic module implements assumes that LinearRing#is_simple? will return false given a self-intersection, but this is inconsistent with older versions of libgeos. To fix this, ProjectedLinearRingImpl#is_simple? will now rely on CAPILinearRingImpl#valid? instead of CAPILinearRingImpl#is_simple? since this is consistent with the Geographic definition.

This will be a breaking change since users with older versions of libgeos will now have more strict validation requirements on projected factories.

@keithdoggett keithdoggett marked this pull request as ready for review November 1, 2020 18:00
@keithdoggett
Copy link
Member Author

The reason for overriding is_simple? in ProjectedLinearRingMethods instead of validate_geometry is because the idea behind projection factories is that they can be almost swappable with the spherical factory. This means that the only real difference should be that the spherical factory uses a spherical math backend while the proejction factories use a PCS as their backend, but the methods should have the same behavior. In this case, is_simple? in the spherical factory was equivalent to valid? in the libgeos API so this will make the behavior between each factory more consistent.

The only issue with this could be if there's a case where a LinearRing is simple but not valid or vice versa. I'll look into this but I think it should be ok.

Copy link
Member

@BuonOmo BuonOmo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, however it is definitely a breaking change. I guess you could add this change to the History.md file and we'll start a dev-3 branch (or the naming your prefer!)

Comment on lines +112 to +122
# having issues with floating point errors on some systems
# 4.3 -> 4.29999999999999, for example, and throws an error
# iterating through points and using assert_in_delta instead
# of assert_equal
b_coords = buffered_line_string.exterior_ring.coordinates
p_coords = polygon.exterior_ring.coordinates
p_coords.zip(b_coords).each do |p_coord, b_coord|
p_coord.zip(b_coord).each do |pt1, pt2|
assert_in_delta(pt1, pt2, 1e-7)
end
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is not satisfying, but this PR is not the place to fix floating point errors!

@@ -15,6 +15,14 @@ def setup
@factory = RGeo::Geographic.simple_mercator_factory
end

def test_is_simple_validation_behavior
# issue 218
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always find it easier to show an url in that case:

Suggested change
# issue 218
# See https://github.com/rgeo/rgeo/issues/218

@keithdoggett keithdoggett changed the base branch from master to v3-dev November 11, 2020 18:20
@keithdoggett keithdoggett merged commit beb4df4 into v3-dev Nov 11, 2020
@keithdoggett keithdoggett deleted the standardize-validation branch November 11, 2020 23:32
keithdoggett added a commit that referenced this pull request Apr 9, 2021
BuonOmo pushed a commit that referenced this pull request Apr 16, 2021
BuonOmo pushed a commit that referenced this pull request Jan 13, 2022
BuonOmo pushed a commit that referenced this pull request Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Different Validation Behavior (is_simple) in Geos 3.8.x
3 participants