Add `is_closed` for MultiLineString and fix `is_closed` for empty LineString #523

michaelkirk · 2020-10-20T19:40:05Z

Some commits extracted from my geomgraph work

michaelkirk · 2020-10-20T19:41:49Z

geo/src/utils.rs

@@ -98,7 +98,7 @@ where
    //
    // See: https://en.wikipedia.org/wiki/Point_in_polygon

-    assert!(linestring.is_closed());
+    debug_assert!(linestring.is_closed());


I'm not sure if this is controversial - we have some "undefined behavior" for invalid geometries, yet we don't (yet) have a way of asserting validity of geometries.

I think we should be a little more conservative about crashing in release builds.

michaelkirk · 2020-10-20T19:44:00Z

geo/src/algorithm/contains/polygon.rs

+        // Officially, we require the exterior to be a valid ring, but this isn't curently enforced
+        // by the Polygon contructor. We bail out here rather than trigger later debug asserts.
+        if self.exterior().0.is_empty() {
+            return false;


This is to avoid hitting the assert in coord_pos_relative_to_ring.

Without this, a unit test exercises that path and fails.

There's arguably other approaches - like deleting that unit test. I think ultimately a more blessed path for validating geometries or perhaps having these method return Result types which return Err where we're currently asserting. But I didn't particularly feel like taking that larger change on yet.

frewsxcv

lgtm! should we consider this a breaking change since it's a change in logic for public methods? i don't feel strongly one way or the other, especially since things are probably gonna be broken in general for folks working with empty linestrings

michaelkirk · 2020-10-20T19:55:55Z

should we consider this a breaking change since it's a change in logic for public methods?

I'd say no, because there's no change to the existing API, and the behavioral change is strictly a bug fix.

And the behavior was even documented, though not implemented:

Close the LineString. Specifically, if the LineString has at least one coordinate, and
the value of the first coordinate does not equal the value of the last coordinate, then a
new coordinate is added to the end with the value of the first coordinate.

michaelkirk · 2020-10-20T19:57:34Z

Though actually I have another PR coming up in a minute which is maybe a big enough new feature to bump the minor version so it's maybe a moot point.

pre-1.0 semver is... "hard" to reason about.

rmanoka · 2020-10-21T04:54:08Z

@michaelkirk the new logic doesn't consider an empty linestring as closed? I think this is not consistent with other libs. See for eg. JTS linear ring which accepts empty ones. Any reason for considering empty as not-closed?

michaelkirk · 2020-10-21T05:32:09Z

@michaelkirk the new logic doesn't consider an empty linestring as closed? I think this is not consistent with other libs. See for eg. JTS linear ring which accepts empty ones. Any reason for considering empty as not-closed?

Thanks for taking a look @rmanoka. That's interesting.

~~I was mostly just going off our own documentation which says:~~ (edit: removed half completed / incorrect thought)

Indeed - JTS allows empty LinearRings, and LinearRing#isClosed considers empty LinearRings as closed:

  public boolean isClosed() {
    if (isEmpty()) {
    	// empty LinearRings are closed by definition
      return true;
    }
    return super.isClosed();
  }

And yet, looking a little deeper, JTS does the opposite for their LineString#isClosed, which considers empty linestrings as not closed.

  public boolean isClosed() {
    if (isEmpty()) {
      return false;
    }
    return getCoordinateN(0).equals2D(getCoordinateN(getNumPoints() - 1));
  }

Since we don't have a LinearRing type, our LineString type sometimes does double duty as LinearRings. I guess I don't yet understand the consequences of having it behave like JTS's LineString or whether it should behave like JTS's LinearRing in all cases...

I'm going to keep looking.

rmanoka · 2020-10-21T06:06:15Z

@michaelkirk Interesting find on JTS LineString. I'm inclined to argue that LineString logic is a bug: SFS defines LinearRing as a closed and simple LineString (so empty is automatically a LinearRing). From SFS defn, I'm also hoping we can get away with LineString also proxying for LinearRing when closed.

Can you think of any reason for requiring an empty LineString that's not a LinearRing?

LineString#is_closed like a LinearRing.

michaelkirk · 2020-10-21T16:55:39Z

@michaelkirk Interesting find on JTS LineString. I'm inclined to argue that LineString logic is a bug: SFS defines LinearRing as a closed and simple LineString (so empty is automatically a LinearRing). From SFS defn, I'm also hoping we can get away with LineString also proxying for LinearRing when closed.

Thanks for your thoughts there. I'm compelled!

Can you think of any reason for requiring an empty LineString that's not a LinearRing?

After digging in more, I can see no reason for an empty LineString to behave differently from a LinearRing. Other than just "thinking about it", mainly my approach was to dig through the JTS code base and see where they used the method call. It seems like it wouldn't change any of the libraries internal functionality (though a unit test which specifically tests this behavior would fail).

Thanks for catching this! PTAL @rmanoka

michaelkirk · 2020-10-21T16:57:50Z

geo-types/src/multi_line_string.rs

+    /// assert!(!MultiLineString::<f32>(vec![]).is_closed());
+    /// ```
+    pub fn is_closed(&self) -> bool {
+        if self.0.is_empty() {


Note this bit @rmanoka - it mirrors JTS's MultiLineString#is_closed, but seems to be on similarly shaky ground.

@michaelkirk Ya, not a fan of returning false if empty here either. Usually, it is mathematically neat to extend predicates on list that are defined as a for-all over elements in the list to be true on the empty list. See for instance std::Iterator::all. Since is_closed is such a predicate, I would have liked it to be true on empty list. I have no idea why JTS chose the other way. There is no MultiLinearRing in JTS, so I don't have a compelling argument either :( .

Ultimately, I can't find a reason why it'd matter, so I'm willing to defer to your intuition here.

michaelkirk · 2020-10-21T20:17:58Z

Ok I think I've addressed all your feedback @rmanoka. PTAL

rmanoka

LGTM! Thanks @michaelkirk

michaelkirk · 2020-10-22T15:35:25Z

bors r=rmanoka

bors · 2020-10-22T15:54:41Z

Build succeeded:

524: Geometry dimensions via HasDimensions trait r=rmanoka a=michaelkirk Some efforts extracted from the geomgraph work that seemed generally useful, and thus worth separating. (I'm also trying to do what I can to keep the upcoming geomgraph PR just a little bit smaller.) *Note this PR represents separate functionality from #523, but it relies on #523 for correct behavior, so I'm targeting #523 as the base branch. Before merging we should update the base to target master.* Co-authored-by: Michael Kirk <[email protected]>

639: Introduce the geomgraph module for DE-9IM Relate trait r=michaelkirk a=michaelkirk - [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md). - [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users. --- Fixes #513, #515 (I'm sorry it's so large) ~~I'm going to leave it as a draft (edit: 🤦 I failed to actually open the PR as a draft) while I wait to merge #636 and #638 and then do some rebasing, but I don't anticipate doing other large changes before review.~~ *update: ready for review!* Here's some of the earlier work in pursuit of this: #514 #516 #523 #524 #538 #552 #561 #611 #628 #629 #636 Primarily, this introduces the geomgraph module for a DE-9IM `Relate` trait. geomgraph implements a topology graph largely inspired by JTS's module of the same name: https://github.com/locationtech/jts/tree/jts-1.18.1/modules/core/src/main/java/org/locationtech/jts/geomgraph You can see some of the reference code if you omit the "REMOVE JTS COMMENTS" commit. In some places the implementation is quite close to the JTS source. The overall "flow" is pretty similar to that of JTS, but in the small, there were some divergences. It's not easy (or desirable) to literally translate a Java codebase making heavy use of inheritance and pointers to rust. Additionally, I chose to take advantage of `Option` and rust's enums with associated data to make some case analysis more explicit. There is a corresponding PR in our [jts-test-runner](georust/jts-test-runner#6) crate which includes the bulk of the tests for the new Relate trait. ## Algorithm Overview This functionality is accessed on geometries, via the `Relate` trait, e.g. `line.relate(point)` which returns a DE-9IM [`IntersectionMatrix`](https://en.wikipedia.org/wiki/DE-9IM#Matrix_model). The `Relate` trait is driven by the `RelateOperation`. The `RelateOperation` builds a `GeometryGraph` for each of the two geometries being related. A `GeometryGraph` is a systematic way to organize the "interesting" parts of a geometry's structure - e.g. where its vertices, lines, and areas lie relative to one another. Once the `RelateOperation` has built the two `GeometryGraph`s, it uses them to efficiently compare the two Geometries's structures, outputting the `IntesectionMatrix`. Co-authored-by: Michael Kirk <[email protected]> Co-authored-by: bors[bot] <26634292+bors[bot]@users.noreply.github.com>

639: Introduce the geomgraph module for DE-9IM Relate trait r=frewsxcv,rmanoka a=michaelkirk - [x] I agree to follow the project's [code of conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md). - [x] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users. --- Fixes #513, #515 (I'm sorry it's so large) ~~I'm going to leave it as a draft (edit: 🤦 I failed to actually open the PR as a draft) while I wait to merge #636 and #638 and then do some rebasing, but I don't anticipate doing other large changes before review.~~ *update: ready for review!* Here's some of the earlier work in pursuit of this: #514 #516 #523 #524 #538 #552 #561 #611 #628 #629 #636 Primarily, this introduces the geomgraph module for a DE-9IM `Relate` trait. geomgraph implements a topology graph largely inspired by JTS's module of the same name: https://github.com/locationtech/jts/tree/jts-1.18.1/modules/core/src/main/java/org/locationtech/jts/geomgraph You can see some of the reference code if you omit the "REMOVE JTS COMMENTS" commit. In some places the implementation is quite close to the JTS source. The overall "flow" is pretty similar to that of JTS, but in the small, there were some divergences. It's not easy (or desirable) to literally translate a Java codebase making heavy use of inheritance and pointers to rust. Additionally, I chose to take advantage of `Option` and rust's enums with associated data to make some case analysis more explicit. There is a corresponding PR in our [jts-test-runner](georust/jts-test-runner#6) crate which includes the bulk of the tests for the new Relate trait. ## Algorithm Overview This functionality is accessed on geometries, via the `Relate` trait, e.g. `line.relate(point)` which returns a DE-9IM [`IntersectionMatrix`](https://en.wikipedia.org/wiki/DE-9IM#Matrix_model). The `Relate` trait is driven by the `RelateOperation`. The `RelateOperation` builds a `GeometryGraph` for each of the two geometries being related. A `GeometryGraph` is a systematic way to organize the "interesting" parts of a geometry's structure - e.g. where its vertices, lines, and areas lie relative to one another. Once the `RelateOperation` has built the two `GeometryGraph`s, it uses them to efficiently compare the two Geometries's structures, outputting the `IntesectionMatrix`. Co-authored-by: Michael Kirk <[email protected]> Co-authored-by: bors[bot] <26634292+bors[bot]@users.noreply.github.com>

michaelkirk added 2 commits October 20, 2020 12:19

is_closed for multiline string

97e9b44

fix is_closed for empty LineString

253cbd0

michaelkirk commented Oct 20, 2020

View reviewed changes

frewsxcv approved these changes Oct 20, 2020

View reviewed changes

update changelog

e7dfb7f

michaelkirk force-pushed the mkirk/is-closed branch from 4a185b2 to e7dfb7f Compare October 20, 2020 20:06

michaelkirk mentioned this pull request Oct 20, 2020

Geometry dimensions via HasDimensions trait #524

Merged

partial revert of 253cbd0 and document decision to treat

a36b7db

LineString#is_closed like a LinearRing.

michaelkirk commented Oct 21, 2020

View reviewed changes

michaelkirk added 3 commits October 21, 2020 10:09

fixup! update changelog

15e4894

per CR: make empty MultiLineString act as closed

7a54f2c

fixup docs

6ad4cbc

update doc test

c6b505c

rmanoka approved these changes Oct 22, 2020

View reviewed changes

bors bot merged commit 55a7ebd into master Oct 22, 2020

michaelkirk deleted the mkirk/is-closed branch October 26, 2020 23:51

michaelkirk mentioned this pull request Mar 12, 2021

Introduce the geomgraph module for DE-9IM Relate trait #639

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `is_closed` for MultiLineString and fix `is_closed` for empty LineString #523

Add `is_closed` for MultiLineString and fix `is_closed` for empty LineString #523

michaelkirk commented Oct 20, 2020

michaelkirk Oct 20, 2020

michaelkirk Oct 20, 2020 •

edited

Loading

frewsxcv left a comment •

edited

Loading

michaelkirk commented Oct 20, 2020 •

edited

Loading

michaelkirk commented Oct 20, 2020

rmanoka commented Oct 21, 2020

michaelkirk commented Oct 21, 2020 •

edited

Loading

rmanoka commented Oct 21, 2020

michaelkirk commented Oct 21, 2020

michaelkirk Oct 21, 2020

rmanoka Oct 21, 2020

michaelkirk Oct 21, 2020

michaelkirk commented Oct 21, 2020

rmanoka left a comment

michaelkirk commented Oct 22, 2020

bors bot commented Oct 22, 2020

Add is_closed for MultiLineString and fix is_closed for empty LineString #523

Add is_closed for MultiLineString and fix is_closed for empty LineString #523

Conversation

michaelkirk commented Oct 20, 2020

michaelkirk Oct 20, 2020

Choose a reason for hiding this comment

michaelkirk Oct 20, 2020 • edited Loading

Choose a reason for hiding this comment

frewsxcv left a comment • edited Loading

Choose a reason for hiding this comment

michaelkirk commented Oct 20, 2020 • edited Loading

michaelkirk commented Oct 20, 2020

rmanoka commented Oct 21, 2020

michaelkirk commented Oct 21, 2020 • edited Loading

rmanoka commented Oct 21, 2020

michaelkirk commented Oct 21, 2020

michaelkirk Oct 21, 2020

Choose a reason for hiding this comment

rmanoka Oct 21, 2020

Choose a reason for hiding this comment

michaelkirk Oct 21, 2020

Choose a reason for hiding this comment

michaelkirk commented Oct 21, 2020

rmanoka left a comment

Choose a reason for hiding this comment

michaelkirk commented Oct 22, 2020

bors bot commented Oct 22, 2020

Add `is_closed` for MultiLineString and fix `is_closed` for empty LineString #523

Add `is_closed` for MultiLineString and fix `is_closed` for empty LineString #523

michaelkirk Oct 20, 2020 •

edited

Loading

frewsxcv left a comment •

edited

Loading

michaelkirk commented Oct 20, 2020 •

edited

Loading

michaelkirk commented Oct 21, 2020 •

edited

Loading