centroid fixups #629

michaelkirk · 2021-02-20T01:47:20Z

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.

Fixes the implementation for a MultiLineString with zero length lines
impl Centroid for Geometry and it's remaining variants.

FYI: I found these via the JTS test suite runner I'm working on here: https://github.com/michaelkirk/jts-test-runner/pull/1/files

Status:

integrate jts-test-runner centroid tests
needs review http://github.com/michaelkirk/jts-test-runner/pull/1
move http://github.com/michaelkirk/jts-test-runner/pull/1 to georust
publish jts-test-runner? (alternatively, we could leave it unpublished and just reference GH for this unique crate)
publish wkt
remove unpublished references from patch.crates-io

frewsxcv

Ran out of time and still need to read through impl<T> Centroid for GeometryCollection<T>, but the rest looks good! Would be great if we had tests for the new implementations, but no worries if you don't have capacity

Cargo.toml

frewsxcv · 2021-02-20T15:12:47Z

geo/src/algorithm/centroid.rs

+                    .iter()
+                    .flat_map(|line_string| Some(Point::from(*line_string.0.first()?)))
+                    .collect();
+                MultiPoint(points).centroid()


Something to explore in the future is extracting the body of the impl Centroid for MultiPoint implementation into a separate function that takes an iterator of Points. Then we can call it here to avoid the Vec allocation

Good point, I've gone ahead and pushed a commit to this PR to achieve this.

geo/src/algorithm/centroid.rs

michaelkirk · 2021-02-20T21:31:19Z

geo/src/algorithm/centroid.rs

+}
+
+fn centroid_of_coords<T>(
+    mut coords_iter: impl Iterator<Item = Coordinate<T>>,


I considered implementing this as impl Centroid for CoordsIter but thought it might be a footgun, since, e.g. Polygon implements CoordsIter, but we'd never want to use the impl CoordsIter for Centroid for a Polygon.

frewsxcv

Ty!

geo/src/algorithm/centroid.rs

frewsxcv · 2021-02-20T22:13:07Z

geo/src/algorithm/centroid.rs

+                (Dimensions::TwoDimensional, Some(centroid)) => {
+                    let mut centroid_accum = centroid_2d_accum.unwrap_or_default();
+
+                    // REVIEW: unsigned?


I think so? It hurts my brain thinking about it

Yeah, this should be unsigned_area. We don't enforce orientations of polygon, and for centroid, the weights can't be negative (it's a convex combination)

I verified that some test cases fail if I switch this to signed_area.

frewsxcv · 2021-02-20T22:16:18Z

geo/src/algorithm/centroid.rs

+                            | Geometry::Polygon(_)
+                            | Geometry::MultiPolygon(_)
+                            | Geometry::Rect(_)
+                            | Geometry::Triangle(_) => T::zero(),


Do we ever expect this branch to get hit? If not, do you have opinions about using unreachable!() here?

It's complicated 😅

For Geometry::Point(_), and Geometry::MultiPoint(_), I'm confident that it should never happen and could add an unreachable.

HasDimensions for Rect and Triangle supports degenerating to 1-D.

impl<C: CoordNum> HasDimensions for Triangle<C> { ... fn dimensions(&self) -> Dimensions { if self.0 == self.1 && self.1 == self.2 { // degenerate triangle is a point Dimensions::ZeroDimensional } else if self.0 == self.1 || self.1 == self.2 || self.2 == self.0 { // degenerate triangle is a line Dimensions::OneDimensional } else { Dimensions::TwoDimensional } } ... }

So I guess it's expected for some inputs.

For Polygon/MultiPolygon... it's certainly impossible for valid Polygons (closed linestring with 3+ points), but am not confident that we'd want to crash a production instance.

How about a debug_assert for those?

How about a debug_assert for those?

Up to you! My preference is to have assertions (or a comment) since it'll indicate to the reader that this is not a branch we expect to get hit, and could surface bugs

michaelkirk · 2021-02-25T23:13:06Z

Ok! I've completed my TODOS:

Status:

integrate jts-test-runner centroid tests
needs review http://github.com/michaelkirk/jts-test-runner/pull/1
move http://github.com/michaelkirk/jts-test-runner/pull/1 to georust
publish jts-test-runner? (alternatively, we could leave it unpublished and just reference GH for this unique crate)
publish wkt
remove unpublished references from patch.crates-io

I rebased, but here are the changes since your last review @frewsxcv:
https://github.com/georust/geo/pull/629/files/4b5ab0862bb06e19501534035414513c9053c1df..c3b3ff2a19a9db14709a708e4a3aeb8436f58675

michaelkirk · 2021-02-25T23:15:24Z

geo/Cargo.toml

@@ -34,6 +34,9 @@ use-serde = ["serde", "geo-types/serde"]
 [dev-dependencies]
 approx = "0.4.0"
 criterion = { version = "0.3" }
+# jts-test-runner is an internal crate which exists only to be part of the geo test suite.
+# As such it's kept unpublished. It's in a separate repo primarily because it's kind of large.
+jts-test-runner = { git = "https://github.com/georust/jts-test-runner", commit = "bf873f7858792498b70451c2818c40d4e0975264" }


This might be controversial... but it seemed like a lot of ceremony to go through the publish dance for something which is basically an internal dependency.

I could be convinced otherwise!

If anything, I'd reconsider just including the crate in the main geo repo - It's 15MB of xml.

One thing to note is that the current cargo package size limit is 10mb…

Ah, good to know! My current solution is indeed to just agglomerate the test fixtures into the jts-test-runner crate binary.

I'm including a pretty small subset at this point, but good to know that we may eventually need to do something else.

Still, I'm now more inclined to leave it as a separate crate.

rmanoka · 2021-02-22T06:01:36Z

geo/src/algorithm/centroid.rs

+                (Dimensions::TwoDimensional, Some(centroid)) => {
+                    let mut centroid_accum = centroid_2d_accum.unwrap_or_default();
+
+                    // REVIEW: unsigned?


Yeah, this should be unsigned_area. We don't enforce orientations of polygon, and for centroid, the weights can't be negative (it's a convex combination)

rmanoka · 2021-02-28T05:38:41Z

geo/Cargo.toml

@@ -34,6 +34,9 @@ use-serde = ["serde", "geo-types/serde"]
 [dev-dependencies]
 approx = "0.4.0"
 criterion = { version = "0.3" }
+# jts-test-runner is an internal crate which exists only to be part of the geo test suite.
+# As such it's kept unpublished. It's in a separate repo primarily because it's kind of large.
+jts-test-runner = { git = "https://github.com/georust/jts-test-runner", commit = "bf873f7858792498b70451c2818c40d4e0975264" }


I'm for leaving it as a separate unpublished crate. The Cargo.toml should be rev = <commit hash> (not commit =)

Whoops, thanks!

rmanoka · 2021-02-28T05:52:49Z

geo/src/algorithm/centroid.rs

+                (Dimensions::Empty, _) | (_, None) => continue,
+                (Dimensions::ZeroDimensional, Some(centroid)) => {
+                    let mut centroid_accum = centroid_0d_accum.unwrap_or_default();
+                    let weight = T::one();


This is inaccurate IMO. Suppose we have a GeomCollection (MultiPoint with 3 points, MultiPoint with 4points), we would want the centroid to be the same as MultiPoint(the 7 points together). However, the above logic will weight them differently.

I think we should extend our Centroid trait to add a method that returns both the centroid Point, and the total weight of the geometry. The weight is number of points, or the total distance, or total area as per the dimension. It should also simplify our code in many cases were we re-compute the weights.

ooph good catch. I'm going to rework this, but might not have time for a few days.

Thank you so much for catching this mistake!

Ok, I believe I've addressed this, but it ended being non-trivial. In the process I found a few more bugs and fleshed out some more tests (almost always the issues were with degenerate geometries).

I think we should extend our Centroid trait to add a method that returns both the centroid Point, and the total weight of the geometry.

I started with this approach, and it would've worked fine, but I felt things ended up cleaner when I bundled the state and logic into a separate private CentroidOperation accumulation struct.

Even in cases where there is a single geometry, we'll still utilize the aggregating operation, which at first seems weird. This is primarily to avoid redundantly having to recompute the centroids weight in the case the geometry does appear in a collection. Since computing the centroid essentially involves computing its weight anyway, there's not much lost by tracking it in this way, except some small extra stack allocations in some cases.

Since I've essentially re-written the trait at this point, it'd be great if I could get another review.

rmanoka

Cool work @michaelkirk ! There's only one minor change I've requested in one of the degeneracy handling logic (1-D triangle vs closed line string).

rmanoka · 2021-03-06T20:37:25Z

geo/src/algorithm/centroid.rs

+    fn add_triangle(&mut self, triangle: &Triangle<T>) {
+        match triangle.dimensions() {
+            ZeroDimensional => self.add_coord(triangle.0),
+            OneDimensional => {


This seems different from how we are handling a degenerate ring. I think we should simply add all three line segments in this case rather than just the longest one. The centroid point itself doesn't change, but the weight would be double what it is now.

These sort of degenerate cases are not really valid as per SFS, so we just have to be internally consistent. Below is a natural test case that users would expect to pass (IMO).

#[test] fn degenerate_triangle_like_ring() { let triangle = Triangle(c(0., 0.), c(1., 1.), c(2., 2.)); let poly: Polygon<_> = triangle.clone().into(); let line = Line::new(c(0., 1.), c(1., 3.)); let g1 = GeometryCollection(vec![triangle.into(), line.into()]); let g2 = GeometryCollection(vec![poly.into(), line.into()]); assert_eq!(g1.centroid(), g2.centroid()); }

Yeah, that's a great point.

Plus your suggestion is simpler. =)

I've rebased and pushed up one new commit to address this for Triangles and Rects (which had the same issue).

…neString is the centroid of it's points

I replaced assert_eq with assert_relative_eq to fix a new failure from a very small deviation in expected results from the new centroid calculation

… MultiLineString

This reverts commit 00df4bd.

Of these Geometry cases: - only Triangle/Rect support collapsing to 1-D - Point/MultiPoint should truly never be 1-D - Polygon/MultiPolygon don't *currently* support being 1-D, but I could imagine a future that similarly allows invalid Polygons to collapse. In that case I'd rather hit a debug_assert and return a reasonable value here than crash someone in production.

Introduces a private CentroidOperation to accumulate results.

michaelkirk · 2021-03-06T22:14:58Z

Thanks for the review @rmanoka!

Since it was a big change, I'm going to leave it open for a couple days before merging in case @frewsxcv or others want to take another look.

michaelkirk · 2021-03-08T15:59:21Z

bors r+

bors · 2021-03-08T16:18:46Z

Build succeeded:

ci result

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 <michael.code@endoftheworl.de> 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 <michael.code@endoftheworl.de> Co-authored-by: bors[bot] <26634292+bors[bot]@users.noreply.github.com>

michaelkirk mentioned this pull request Feb 20, 2021

JTS TestRunner which supports most Centroid and ConvexHull cases. georust/jts-test-runner#1

Merged

frewsxcv reviewed Feb 20, 2021

View reviewed changes

michaelkirk commented Feb 20, 2021

View reviewed changes

frewsxcv approved these changes Feb 20, 2021

View reviewed changes

michaelkirk force-pushed the mkirk/centroid-fixups branch 2 times, most recently from 6da8207 to 7343b5d Compare February 23, 2021 19:14

michaelkirk mentioned this pull request Feb 23, 2021

WKT Release: proposed minor release 0.9.1 georust/wkt#63

Closed

michaelkirk force-pushed the mkirk/centroid-fixups branch 2 times, most recently from 61eb102 to c3b3ff2 Compare February 25, 2021 23:09

michaelkirk commented Feb 25, 2021

View reviewed changes

michaelkirk requested a review from rmanoka February 27, 2021 17:12

rmanoka reviewed Feb 28, 2021

View reviewed changes

michaelkirk mentioned this pull request Mar 2, 2021

Add test for Intersects predicate georust/jts-test-runner#5

Merged

michaelkirk force-pushed the mkirk/centroid-fixups branch 6 times, most recently from eb3c24b to d744ee8 Compare March 5, 2021 19:18

rmanoka approved these changes Mar 6, 2021

View reviewed changes

michaelkirk added 9 commits March 6, 2021 13:10

always use local geo versions when working in workspace

7d6415b

add missing centroid impls

bf42f97

per JTS test suite: the centroid of a dimensionally collapsed MultiLi…

a777401

…neString is the centroid of it's points

Fix GeometryCollection centroid

6568d13

update tests

35e1b2b

I replaced assert_eq with assert_relative_eq to fix a new failure from a very small deviation in expected results from the new centroid calculation

update CHANGES

9436b7b

avoid allocating for MultiPoint when computing centroid of degenerate…

4ff80e3

… MultiLineString

Revert "always use local geo versions when working in workspace"

c1e04bf

This reverts commit 00df4bd.

code comments / algo overview

c6372a4

michaelkirk added 8 commits March 6, 2021 13:10

integrate jts-test-runner

decc0fa

verified some JTS test cases fail if I switch to signed area

bd45959

proper syntax for referencing a commit in crates patches

98c5415

use local crates for transitive deps too

087b133

add failing test case

f644cb4

Properly weight compound geometries

60dd0e6

Introduces a private CentroidOperation to accumulate results.

fixup off-axis degenerate triangles

4a61b73

michaelkirk force-pushed the mkirk/centroid-fixups branch from 2ff2acf to 9bc5c00 Compare March 6, 2021 21:19

treat 1-d degenerate triangles/rects the same as a degenerate 1-d ring

9187c86

michaelkirk force-pushed the mkirk/centroid-fixups branch from 9bc5c00 to 9187c86 Compare March 6, 2021 21:29

bors bot merged commit 6e37af2 into master Mar 8, 2021

michaelkirk mentioned this pull request Mar 12, 2021

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

Merged

2 tasks

michaelkirk deleted the mkirk/centroid-fixups branch April 16, 2021 19:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

centroid fixups #629

centroid fixups #629

michaelkirk commented Feb 20, 2021 •

edited

Loading

frewsxcv left a comment

frewsxcv Feb 20, 2021

michaelkirk Feb 20, 2021

michaelkirk Feb 20, 2021

frewsxcv left a comment

frewsxcv Feb 20, 2021

rmanoka Feb 22, 2021

michaelkirk Feb 25, 2021

frewsxcv Feb 20, 2021

michaelkirk Feb 21, 2021

frewsxcv Feb 22, 2021

michaelkirk commented Feb 25, 2021

michaelkirk Feb 25, 2021

michaelkirk Feb 25, 2021

urschrei Feb 27, 2021

michaelkirk Feb 27, 2021 •

edited

Loading

rmanoka Feb 22, 2021

rmanoka Feb 28, 2021

michaelkirk Feb 28, 2021

rmanoka Feb 28, 2021

michaelkirk Feb 28, 2021 •

edited

Loading

michaelkirk Mar 5, 2021

rmanoka left a comment

rmanoka Mar 6, 2021

michaelkirk Mar 6, 2021

michaelkirk commented Mar 6, 2021

michaelkirk commented Mar 8, 2021

bors bot commented Mar 8, 2021

centroid fixups #629

centroid fixups #629

Conversation

michaelkirk commented Feb 20, 2021 • edited Loading

frewsxcv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frewsxcv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkirk commented Feb 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkirk Feb 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkirk Feb 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmanoka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkirk commented Mar 6, 2021

michaelkirk commented Mar 8, 2021

bors bot commented Mar 8, 2021

michaelkirk commented Feb 20, 2021 •

edited

Loading

michaelkirk Feb 27, 2021 •

edited

Loading

michaelkirk Feb 28, 2021 •

edited

Loading