Improve MultiPolygon centroid robustness #118

dbaston · 2017-08-19T14:40:20Z

@jnh5y as we discussed the other day: this PR improves the robustness of centroid computations for MultiPolygons with disparate components. It is a very late resolution to the topic discussed in this thread: https://sourceforge.net/p/jts-topo-suite/mailman/message/31595183/

jnh5y · 2017-08-19T14:55:02Z

@dbaston great! There are a few things to iron out around contributing to an Eclipse project. From the https://github.com/locationtech/jts/blob/master/CONTRIBUTING.md, you'll need to sign the ECA (https://www.eclipse.org/legal/ECA.php) and 'sign' each git commit with 'git commit -s'.

jnh5y · 2017-08-19T14:57:25Z

modules/core/src/main/java/org/locationtech/jts/algorithm/Centroid.java

@@ -153,7 +153,6 @@ else if (ptCount > 0){

  private void setBasePoint(Coordinate basePt)
  {
-    if (this.areaBasePt == null)


interesting This fixes the issue?

For the test case I included, yes.

jnh5y

Dan and I chatted about this in person at the FOSS4G Boston code sprint.

The key issue is that multipolygons should reset the basePoint for each component.

jnh5y · 2017-08-19T16:01:04Z

@dr-jts, any input on this? Looks good to me. Dan explained it in person and it sounds reasonable.

Do this by allowing each polygon shell to set its own origin point, instead of re-using the origin point from the first polygon in the geometry. Signed-off-by: Daniel Baston <[email protected]>

dr-jts · 2017-08-21T21:16:40Z

The fix looks good. I am going to make some comments on the PR.

@dbaston are you going to fill out an ECA? Seems worthwhile, since you've been so productive with JTS enhancements.

dr-jts · 2017-08-21T21:21:32Z

modules/core/src/test/java/org/locationtech/jts/algorithm/CentroidTest.java

+            cy += areaFraction * componentCentroid.y;
+        }
+
+        assertEquals(new Coordinate(cx, cy), g.getCentroid().getCoordinate());


This seems highly susceptible to round-off issues. Does it really work in this case?

Would be better to use a precision tolerance in the comparison, I think.

Also would be good to factor the area-weighted algorithm out into a separate method, both to highlight that it is an alternative algorithm to the current polygon centroid computation, and to allow reusing it for other test cases. (Given this nice addition of a Centroid unit test, it would be nice to add more complete coverage to the tests it contains. Also, that would provide better coverage of the area-weighted algorithm - which of course also needs to be "proven out" by some testing!)

I can factor the weighted-average centroid out for use in other tests, but the intent wasn't to propose it as a separate algorithm for general use. The intent with this test was only to show that the change to the base algorithm (moving the triangle base point for each polygon shell) produces results that are equivalent to the area-weighted average described in the linked email chain.

The results satisfy Coordinate::equals in this case but may suffer from round-off discrepancies with other inputs. I can change the assertion to check that the distance between the two centroids is < 1e-14 or something, if that is preferable.

I just mean factoring it out as a method in the CentroidTest class. That shows that it is useful for building tests, but doesn't promote using it for actual computation.

Adding a tolerance would be great. Note that Coordinate provides an equals2D-with-tolerance method.

Sounds good. I've made these changes.

dr-jts · 2017-08-21T21:25:24Z

modules/tests/src/test/resources/testxml/general/TestCentroid.xml

+      -92.65512450000065 36.586800000000466
+    )))
+  </a>
+  <test><op name="getCentroid" arg1="A" >POINT (-92.6553838608954 36.58695407733924)</op></test>


A similar comment here. This seems brittle WRT numeric precision. Not your problem, but it would be nice if the XML test syntax allowed defining a precision tolerance for expected results. (For centroid computation the tolerance can be quite fine, since the algorithm should produce very close results).

Since the XML Test format doesn't provide a way of expressing a tolerance value, this is fine as it stands. If it breaks in the future, that will be motivation to add a tolerance!

dr-jts · 2017-08-21T22:25:29Z

@dbaston would the same robustness issue occur if the polygons in the example were holes in a larger polygon? In this case I suspect the area base point should be reset for each ring?

Signed-off-by: Daniel Baston <[email protected]>

dbaston · 2017-08-22T02:08:34Z

Yes, it intuitively seems beneficial to reset the base point for each ring. I don't have a test case showing failure without doing this, but I can make the change anyway if you like.

Oh, and yes, I filled out an ECA during the sprint on Saturday.

dr-jts · 2017-08-22T04:02:28Z

modules/core/src/test/java/org/locationtech/jts/algorithm/CentroidTest.java

+
+public class CentroidTest extends TestCase {
+
+    private static final double tolerance = 1e-10;


A minor style point - I like to make static final fieldnames uppercase, so it's obvious that they are constants.

dr-jts · 2017-08-22T04:06:03Z

@dbaston re setting base point for holes: on further reflection I suspect that the computation would be dominated by the larger enclosing shell, so that there's probably no need to worry about the affect of small holes. Without an example demonstrating the issue there's no need to make a fix for this.

dr-jts · 2017-08-22T04:08:01Z

modules/core/src/main/java/org/locationtech/jts/algorithm/Centroid.java

@@ -153,7 +153,6 @@ else if (ptCount > 0){

  private void setBasePoint(Coordinate basePt)


How about we rename this to setAreaBasePoint? Makes it a teeny bit more descriptive, and indicates that this functionality is only necessary for areas.

jnh5y reviewed Aug 19, 2017

View reviewed changes

dbaston force-pushed the centroid-robustness branch from 1c470e5 to 44dbbfb Compare August 19, 2017 15:27

jnh5y approved these changes Aug 19, 2017

View reviewed changes

Improve MultiPolygon centroid robustness

36b8ae3

Do this by allowing each polygon shell to set its own origin point, instead of re-using the origin point from the first polygon in the geometry. Signed-off-by: Daniel Baston <[email protected]>

dbaston force-pushed the centroid-robustness branch from 44dbbfb to 36b8ae3 Compare August 19, 2017 16:50

dr-jts reviewed Aug 21, 2017

View reviewed changes

dr-jts added type-bug jts-core labels Aug 21, 2017

Refactor test case

ce19c0a

Signed-off-by: Daniel Baston <[email protected]>

dr-jts reviewed Aug 22, 2017

View reviewed changes

dbaston added 2 commits August 22, 2017 05:53

Rename method for clarity

317365b

Fix formatting of tolerance constant

9e71b7d

dr-jts merged commit b430ca9 into locationtech:master Aug 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve MultiPolygon centroid robustness #118

Improve MultiPolygon centroid robustness #118

dbaston commented Aug 19, 2017

jnh5y commented Aug 19, 2017

jnh5y Aug 19, 2017

dbaston Aug 19, 2017

jnh5y left a comment

jnh5y commented Aug 19, 2017

dr-jts commented Aug 21, 2017

dr-jts Aug 21, 2017

dbaston Aug 21, 2017

dr-jts Aug 21, 2017

dbaston Aug 22, 2017

dr-jts Aug 21, 2017

dr-jts Aug 21, 2017

dr-jts commented Aug 21, 2017

dbaston commented Aug 22, 2017 •

edited

Loading

dr-jts Aug 22, 2017

dr-jts commented Aug 22, 2017

dr-jts Aug 22, 2017


		public class CentroidTest extends TestCase {

		private static final double tolerance = 1e-10;

		@@ -153,7 +153,6 @@ else if (ptCount > 0){

		private void setBasePoint(Coordinate basePt)

Improve MultiPolygon centroid robustness #118

Improve MultiPolygon centroid robustness #118

Conversation

dbaston commented Aug 19, 2017

jnh5y commented Aug 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnh5y left a comment

Choose a reason for hiding this comment

jnh5y commented Aug 19, 2017

dr-jts commented Aug 21, 2017

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

dr-jts commented Aug 21, 2017

dbaston commented Aug 22, 2017 • edited Loading

Choose a reason for hiding this comment

dr-jts commented Aug 22, 2017

Choose a reason for hiding this comment

dbaston commented Aug 22, 2017 •

edited

Loading