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

Dealing with Validity #123

Closed
urschrei opened this issue Jun 23, 2017 · 3 comments · Fixed by #1279
Closed

Dealing with Validity #123

urschrei opened this issue Jun 23, 2017 · 3 comments · Fixed by #1279

Comments

@urschrei
Copy link
Member

See previous discussions in #56 and #122
First, what do we mean when we talk about validity? The OGC OpenGIS spec discussed in the PostGIS docs gives a good overview.

  • It's probably not desirable to have validating constructors by default, as validation can incur a significant performance overhead, but they should probably be available, either as separate constructors, or with a validate=True argument, although the latter could cause a lot of headaches in terms of revising calls to the existing constructors, and dealing with invalid geometries; e.g. both of the simplification algorithms can return invalid geometries, so those traits would always have to return a Result (they should arguably already be doing this)
  • Should we check for validity before carrying out certain operations? This is what Shapely does, raising an error if invalid geometries are detected. I'm not sure what JTS / GEOS do.
  • I think the two points above directly imply that every geometry operation must return a Result.
@frewsxcv
Copy link
Member

Regardless of when we check validity of geometry, we'll need to implement the validity checks before that happens. Unless anyone else has a better suggestion, I think we can use the same validity checks described by OGC, mentioned in the original post above. Opening a new issue for this: #127

@Michael-F-Bryan
Copy link
Contributor

I like the concept of "making illegal states unrepresentable". That way you can do things knowing that a particular assumption you make (e.g. all Polygons are closed) will always be valid.

These checks can often introduce O(n) or O(n^2) slow downs though, so what about adding validity checks which are only enabled in debug builds so all issues are found during development without hurting the performance of production code?

For example, you might implement the Polygon::new() function like this:

impl<F: Float> Polygon<F> {
  fn new(exterior: LineString<F>, interior: Vec<LineString<F>>) -> Polygon<F> {
    let poly = Polygon { exterior, interior };
    debug_assert!(poly.is_valid(), "All rings and lines in a polygon must be closed");
    poly
  }
  
  fn is_valid(&self) -> bool {
    exterior.is_closed() && interior.iter().all(|ring| ring.is_closed())
  }
}

Another option would be to add new_checked() constructors which create a new shape and then call is_valid() to make sure it's valid.


One question that comes to mind is that if there's a way for someone to create a shape which could break assumptions (e.g. making a polygon which hasn't got a closed exterior), should that constructor be marked unsafe?

@frewsxcv
Copy link
Member

For posterity, there was a discussion about validity of Multi geometry types, and we agreed that empty Multi types are considered valid #444

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants