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

Add validity check per method and validity API #276

Merged
merged 30 commits into from
Jan 13, 2022

Conversation

BuonOmo
Copy link
Member

@BuonOmo BuonOmo commented Oct 19, 2021

Summary

Now that we can handle validity with much more finesse, we can leverage that to have a per method validity check.

Details

This PR adds the ValidityCheck implementation helper which gives the API below:

  • make_valid is a best effort to return a valid version of the given polygon
  • invalid_reason gives nil if valid, or a string indicating invalid reason
  • check_validity! raises the invalid_reason string if invalid

This module also has a CHECKED_METHODS constant which gives every methods that will be overriden after implementation to make sure they check validity before being applied.

TODO

  • Complete the CHECKED_METHODS list
  • Check if the ImplHelper::ValidityCheck.override_classes is not too complicated (@keithdoggett I'd love your opinion on that one!)
  • Maybe add some tests
  • And the ValidityCheck module to more implementations:
    • ...
  • fix tests
  • create documentation related to validity changes
  • update history file

@BuonOmo BuonOmo force-pushed the validity-check-cherry-picked branch 2 times, most recently from 01de693 to a4324a3 Compare October 21, 2021 09:22
@keithdoggett
Copy link
Member

@BuonOmo I was able to fix the tests locally by adding a few more methods to UNCHECKED_METHODS. Looks like the CI is still broken though

@BuonOmo
Copy link
Member Author

BuonOmo commented Nov 10, 2021

@keithdoggett thanks! Yeah the FFI issue is still here. I'll try and find some time in the next two weeks to fix this.

FYI I just tried something given there, yet it did not work...

Some other leads:

@keithdoggett
Copy link
Member

@BuonOmo the macos errors are due to homebrew releasing GEOS 3.10. There were some updates to the validity handling (https://libgeos.org/posts/2021-10-01-geos-3-10-released/) and this caused some of the error messages to change. We might have to handle those tests inside the Geos/FFI specific test files and base the error on GEOS version.

I can take a look at that in the #275 branch since it is more related to that.

Assuming that is handled in #275, what else needs to be done for this PR? Documentation?

@BuonOmo
Copy link
Member Author

BuonOmo commented Dec 3, 2021

@BuonOmo the macos errors are due to homebrew releasing GEOS 3.10. There were some updates to the validity handling (libgeos.org/posts/2021-10-01-geos-3-10-released) and this caused some of the error messages to change. We might have to handle those tests inside the Geos/FFI specific test files and base the error on GEOS version.

I can take a look at that in the #275 branch since it is more related to that.

Assuming that is handled in #275, what else needs to be done for this PR? Documentation?

Yes mostly documentation, as stated in private, I think we can merge that and open new issues / PRs to have short commits and allow external contribution for the few bits that are needed

@@ -19,7 +19,7 @@ def projection
end

def envelope
factory.unproject(projection.envelope)
factory.unproject(projection.unsafe_envelope)
Copy link
Member

Choose a reason for hiding this comment

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

What's the reasoning for us having to use the unsafe versions of methods in the projected features?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we call the safe versions, we have some issues:

  • first we end up running check_validity multiple times,
  • second if one wants to call for instance proj_factory.unsafe_area, it will itself call projection.area, which checks for validity.

@keithdoggett
Copy link
Member

@BuonOmo looks like we have to define the unsafe_point_on_surface method to pass all the tests. The other failing test cases should be handled by the GEOS 3.10 update I made in the validity-handling branch.

keithdoggett and others added 7 commits January 13, 2022 17:18
- ensure projection uses `unsafe_` methods, the validity check is
  already done in the main method
- only use utf8 strings to avoid confusion for users (there may be
  more to track)
- make validity_tests a bit more generic
@BuonOmo BuonOmo force-pushed the validity-check-cherry-picked branch from 1b76c2c to e1374fd Compare January 13, 2022 16:19
@BuonOmo
Copy link
Member Author

BuonOmo commented Jan 13, 2022

@keithdoggett actually, the issue is that it is called on MultiPoint, and per spec, it has no point_on_surface method (ex: http://rgeo.wikience.org/pdf/specs/06-103r4_Implementation_Specification_for_Geographic_Information_-_Simple_feature_access_-_Part_1_Common_Architecture_v1.2.1.pdf). Hence I'd rather be in favor of removing the method when used w. multipoints (it doesn't make much sense anyway, just take a random point)

EDIT: this is another harsh one, in fact postgis accepts it everywhere (#205) since we're in ruby and in ruby everything should just work i'd say we make it feature standard, explaining that choice bcause of postgis. Ok for you?

@keithdoggett
Copy link
Member

Ahh yes, good catch. I agree that we should add it to GeometryCollection. PostGIS/GEOS compatibility is probably more important than strict OGC alignment.

@BuonOmo BuonOmo closed this Jan 13, 2022
@keithdoggett keithdoggett reopened this Jan 13, 2022
@BuonOmo BuonOmo merged commit fcde425 into validity-handling Jan 13, 2022
@BuonOmo BuonOmo deleted the validity-check-cherry-picked branch January 13, 2022 17:34
BuonOmo added a commit that referenced this pull request Jan 13, 2022
Co-authored-by: Keith Doggett <keith.doggett887@gmail.com>
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.

None yet

2 participants