From 4f0d6404e1fcab64eaebf0a2aec9ae11b86b104d Mon Sep 17 00:00:00 2001 From: Tal Levy Date: Tue, 18 Feb 2020 19:26:02 -0800 Subject: [PATCH 1/3] Fix geo_shape centroid calculation for downgraded shapes This commit modifies the centroid-calculator/dimensional-shape-type to properly support the instances of polygons that have no area and lines that have no length. Beforehand N/A were returned for the centroid values, but it is best to downcast the shape type to the appropriate type. Closes #52303. --- .../common/geo/CentroidCalculator.java | 231 ++++++++++----- .../common/geo/DimensionalShapeType.java | 134 +-------- .../metrics/GeoCentroidAggregator.java | 2 +- .../common/geo/CentroidCalculatorTests.java | 264 +++++++++++++++++- .../common/geo/DimensionalShapeTypeTests.java | 12 +- .../common/geo/TriangleTreeTests.java | 14 +- .../metrics/GeoCentroidAggregatorTests.java | 7 +- 7 files changed, 425 insertions(+), 239 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/geo/CentroidCalculator.java b/server/src/main/java/org/elasticsearch/common/geo/CentroidCalculator.java index 8df516dbab13f..83ff5cad38d70 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/CentroidCalculator.java +++ b/server/src/main/java/org/elasticsearch/common/geo/CentroidCalculator.java @@ -31,6 +31,11 @@ import org.elasticsearch.geometry.Point; import org.elasticsearch.geometry.Polygon; import org.elasticsearch.geometry.Rectangle; +import org.elasticsearch.search.aggregations.metrics.CompensatedSum; + +import static org.elasticsearch.common.geo.DimensionalShapeType.LINE; +import static org.elasticsearch.common.geo.DimensionalShapeType.POINT; +import static org.elasticsearch.common.geo.DimensionalShapeType.POLYGON; /** * This class keeps a running Kahan-sum of coordinates @@ -38,22 +43,33 @@ * as the centroid of a shape. */ public class CentroidCalculator { - private double compX; - private double compY; - private double sumX; - private double sumY; - private double sumWeight; + CompensatedSum compSumX; + CompensatedSum compSumY; + CompensatedSum compSumWeight; + + private CentroidCalculatorVisitor visitor; private DimensionalShapeType dimensionalShapeType; public CentroidCalculator(Geometry geometry) { - this.sumX = 0.0; - this.compX = 0.0; - this.sumY = 0.0; - this.compY = 0.0; - this.sumWeight = 0.0; - CentroidCalculatorVisitor visitor = new CentroidCalculatorVisitor(this); + this.compSumX = new CompensatedSum(0, 0); + this.compSumY = new CompensatedSum(0, 0); + this.compSumWeight = new CompensatedSum(0, 0); + this.dimensionalShapeType = null; + this.visitor = new CentroidCalculatorVisitor(this); geometry.visit(visitor); - this.dimensionalShapeType = DimensionalShapeType.forGeometry(geometry); + this.dimensionalShapeType = visitor.calculator.dimensionalShapeType; + } + + private CompensatedSum getCompSumX() { + return compSumX; + } + + private CompensatedSum getCompSumY() { + return compSumY; + } + + private CompensatedSum getCompSumWeight() { + return compSumWeight; } /** @@ -63,18 +79,19 @@ public CentroidCalculator(Geometry geometry) { * @param y the y-coordinate of the point * @param weight the associated weight of the coordinate */ - private void addCoordinate(double x, double y, double weight) { - double correctedX = weight * x - compX; - double newSumX = sumX + correctedX; - compX = (newSumX - sumX) - correctedX; - sumX = newSumX; - - double correctedY = weight * y - compY; - double newSumY = sumY + correctedY; - compY = (newSumY - sumY) - correctedY; - sumY = newSumY; - - sumWeight += weight; + private void addCoordinate(double x, double y, double weight, DimensionalShapeType dimensionalShapeType) { + if (this.dimensionalShapeType == null || this.dimensionalShapeType == dimensionalShapeType) { + compSumX.add(x * weight); + compSumY.add(y * weight); + compSumWeight.add(weight); + this.dimensionalShapeType = dimensionalShapeType; + } else if (dimensionalShapeType.compareTo(this.dimensionalShapeType) > 0) { + // reset counters + compSumX.reset(x * weight, 0); + compSumY.reset(y * weight, 0); + compSumWeight.reset(weight, 0); + this.dimensionalShapeType = dimensionalShapeType; + } } /** @@ -86,16 +103,17 @@ private void addCoordinate(double x, double y, double weight) { * @param otherCalculator the other centroid calculator to add from */ public void addFrom(CentroidCalculator otherCalculator) { - int compared = DimensionalShapeType.COMPARATOR.compare(dimensionalShapeType, otherCalculator.dimensionalShapeType); + int compared = dimensionalShapeType.compareTo(otherCalculator.dimensionalShapeType); if (compared < 0) { - sumWeight = otherCalculator.sumWeight; dimensionalShapeType = otherCalculator.dimensionalShapeType; - sumX = otherCalculator.sumX; - sumY = otherCalculator.sumY; - compX = otherCalculator.compX; - compY = otherCalculator.compY; + this.compSumX = otherCalculator.compSumX; + this.compSumY = otherCalculator.compSumY; + this.compSumWeight = otherCalculator.compSumWeight; + } else if (compared == 0) { - addCoordinate(otherCalculator.sumX, otherCalculator.sumY, otherCalculator.sumWeight); + this.compSumX.add(otherCalculator.getCompSumX().value()); + this.compSumY.add(otherCalculator.getCompSumY().value()); + this.compSumWeight.add(otherCalculator.getCompSumWeight().value()); } // else (compared > 0) do not modify centroid calculation since otherCalculator is of lower dimension than this calculator } @@ -104,7 +122,7 @@ public void addFrom(CentroidCalculator otherCalculator) { */ public double getX() { // normalization required due to floating point precision errors - return GeoUtils.normalizeLon(sumX / sumWeight); + return GeoUtils.normalizeLon(getCompSumX().value() / getCompSumWeight().value()); } /** @@ -112,14 +130,14 @@ public double getX() { */ public double getY() { // normalization required due to floating point precision errors - return GeoUtils.normalizeLat(sumY / sumWeight); + return GeoUtils.normalizeLat(getCompSumY().value() / getCompSumWeight().value()); } /** * @return the sum of all the weighted coordinates summed in the calculator */ public double sumWeight() { - return sumWeight; + return getCompSumWeight().value(); } /** @@ -152,62 +170,34 @@ public Void visit(GeometryCollection collection) { @Override public Void visit(Line line) { - // a line's centroid is calculated by summing the center of each - // line segment weighted by the line segment's length in degrees - for (int i = 0; i < line.length() - 1; i++) { - double diffX = line.getX(i) - line.getX(i + 1); - double diffY = line.getY(i) - line.getY(i + 1); - double x = (line.getX(i) + line.getX(i + 1)) / 2; - double y = (line.getY(i) + line.getY(i + 1)) / 2; - calculator.addCoordinate(x, y, Math.sqrt(diffX * diffX + diffY * diffY)); + if (calculator.dimensionalShapeType != POLYGON) { + visitLine(line.length(), line::getX, line::getY); } return null; } + @Override public Void visit(LinearRing ring) { throw new IllegalArgumentException("invalid shape type found [LinearRing] while calculating centroid"); } - private Void visit(LinearRing ring, boolean isHole) { - // implementation of calculation defined in - // https://www.seas.upenn.edu/~sys502/extra_materials/Polygon%20Area%20and%20Centroid.pdf - // - // centroid of a ring is a weighted coordinate based on the ring's area. - // the sign of the area is positive for the outer-shell of a polygon and negative for the holes - - int sign = isHole ? -1 : 1; - double totalRingArea = 0.0; - for (int i = 0; i < ring.length() - 1; i++) { - totalRingArea += (ring.getX(i) * ring.getY(i + 1)) - (ring.getX(i + 1) * ring.getY(i)); - } - totalRingArea = totalRingArea / 2; - - double sumX = 0.0; - double sumY = 0.0; - for (int i = 0; i < ring.length() - 1; i++) { - double twiceArea = (ring.getX(i) * ring.getY(i + 1)) - (ring.getX(i + 1) * ring.getY(i)); - sumX += twiceArea * (ring.getX(i) + ring.getX(i + 1)); - sumY += twiceArea * (ring.getY(i) + ring.getY(i + 1)); - } - double cX = sumX / (6 * totalRingArea); - double cY = sumY / (6 * totalRingArea); - calculator.addCoordinate(cX, cY, sign * Math.abs(totalRingArea)); - - return null; - } @Override public Void visit(MultiLine multiLine) { - for (Line line : multiLine) { - visit(line); + if (calculator.getDimensionalShapeType() != POLYGON) { + for (Line line : multiLine) { + visit(line); + } } return null; } @Override public Void visit(MultiPoint multiPoint) { - for (Point point : multiPoint) { - visit(point); + if (calculator.getDimensionalShapeType() == null || calculator.getDimensionalShapeType() == POINT) { + for (Point point : multiPoint) { + visit(point); + } } return null; } @@ -222,16 +212,39 @@ public Void visit(MultiPolygon multiPolygon) { @Override public Void visit(Point point) { - calculator.addCoordinate(point.getX(), point.getY(), 1.0); + if (calculator.getDimensionalShapeType() == null || calculator.getDimensionalShapeType() == POINT) { + visitPoint(point.getX(), point.getY()); + } return null; } @Override public Void visit(Polygon polygon) { - visit(polygon.getPolygon(), false); + // check area of polygon + + double[] centroidX = new double[1 + polygon.getNumberOfHoles()]; + double[] centroidY = new double[1 + polygon.getNumberOfHoles()]; + double[] weight = new double[1 + polygon.getNumberOfHoles()]; + visitLinearRing(polygon.getPolygon().length(), polygon.getPolygon()::getX, polygon.getPolygon()::getY, false, + centroidX, centroidY, weight, 0); for (int i = 0; i < polygon.getNumberOfHoles(); i++) { - visit(polygon.getHole(i), true); + visitLinearRing(polygon.getHole(i).length(), polygon.getHole(i)::getX, polygon.getHole(i)::getY, true, + centroidX, centroidY, weight, i + 1); } + + double sumWeight = 0; + for (double w : weight) { + sumWeight += w; + } + + if (sumWeight == 0 && calculator.dimensionalShapeType != POLYGON) { + visitLine(polygon.getPolygon().length(), polygon.getPolygon()::getX, polygon.getPolygon()::getY); + } else { + for (int i = 0; i < 1 + polygon.getNumberOfHoles(); i++) { + calculator.addCoordinate(centroidX[i], centroidY[i], weight[i], POLYGON); + } + } + return null; } @@ -241,9 +254,73 @@ public Void visit(Rectangle rectangle) { double sumY = rectangle.getMaxY() + rectangle.getMinY(); double diffX = rectangle.getMaxX() - rectangle.getMinX(); double diffY = rectangle.getMaxY() - rectangle.getMinY(); - calculator.addCoordinate(sumX / 2, sumY / 2, Math.abs(diffX * diffY)); + if (diffX != 0 && diffY != 0) { + calculator.addCoordinate(sumX / 2, sumY / 2, Math.abs(diffX * diffY), POLYGON); + } else if (diffX != 0) { + calculator.addCoordinate(sumX / 2, rectangle.getMinY(), diffX, LINE); + } else if (diffY != 0) { + calculator.addCoordinate(rectangle.getMinX(), sumY / 2, diffY, LINE); + } else { + visitPoint(rectangle.getMinX(), rectangle.getMinY()); + } return null; } + + + private void visitPoint(double x, double y) { + calculator.addCoordinate(x, y, 1.0, POINT); + } + + private void visitLine(int length, CoordinateSupplier x, CoordinateSupplier y) { + // check line has length + double originDiffX = x.get(0) - x.get(1); + double originDiffY = y.get(0) - y.get(1); + if (originDiffX != 0 || originDiffY != 0) { + // a line's centroid is calculated by summing the center of each + // line segment weighted by the line segment's length in degrees + for (int i = 0; i < length - 1; i++) { + double diffX = x.get(i) - x.get(i + 1); + double diffY = y.get(i) - y.get(i + 1); + double xAvg = (x.get(i) + x.get(i + 1)) / 2; + double yAvg = (y.get(i) + y.get(i + 1)) / 2; + double weight = Math.sqrt(diffX * diffX + diffY * diffY); + calculator.addCoordinate(xAvg, yAvg, weight, LINE); + } + } else { + visitPoint(x.get(0), y.get(0)); + } + } + + private void visitLinearRing(int length, CoordinateSupplier x, CoordinateSupplier y, boolean isHole, + double[] centroidX, double[] centroidY, double[] weight, int idx) { + // implementation of calculation defined in + // https://www.seas.upenn.edu/~sys502/extra_materials/Polygon%20Area%20and%20Centroid.pdf + // + // centroid of a ring is a weighted coordinate based on the ring's area. + // the sign of the area is positive for the outer-shell of a polygon and negative for the holes + + int sign = isHole ? -1 : 1; + double totalRingArea = 0.0; + for (int i = 0; i < length - 1; i++) { + totalRingArea += (x.get(i) * y.get(i + 1)) - (x.get(i + 1) * y.get(i)); + } + totalRingArea = totalRingArea / 2; + + double sumX = 0.0; + double sumY = 0.0; + for (int i = 0; i < length - 1; i++) { + double twiceArea = (x.get(i) * y.get(i + 1)) - (x.get(i + 1) * y.get(i)); + sumX += twiceArea * (x.get(i) + x.get(i + 1)); + sumY += twiceArea * (y.get(i) + y.get(i + 1)); + } + centroidX[idx] = sumX / (6 * totalRingArea); + centroidY[idx] = sumY / (6 * totalRingArea); + weight[idx] = sign * Math.abs(totalRingArea); + } } + @FunctionalInterface + private interface CoordinateSupplier { + double get(int idx); + } } diff --git a/server/src/main/java/org/elasticsearch/common/geo/DimensionalShapeType.java b/server/src/main/java/org/elasticsearch/common/geo/DimensionalShapeType.java index db55ee4915c96..ce9ce5a3a9d79 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/DimensionalShapeType.java +++ b/server/src/main/java/org/elasticsearch/common/geo/DimensionalShapeType.java @@ -20,22 +20,9 @@ import org.apache.lucene.store.ByteArrayDataInput; import org.apache.lucene.store.ByteBuffersDataOutput; -import org.elasticsearch.geometry.Circle; -import org.elasticsearch.geometry.Geometry; import org.elasticsearch.geometry.GeometryCollection; -import org.elasticsearch.geometry.GeometryVisitor; -import org.elasticsearch.geometry.Line; -import org.elasticsearch.geometry.LinearRing; -import org.elasticsearch.geometry.MultiLine; -import org.elasticsearch.geometry.MultiPoint; -import org.elasticsearch.geometry.MultiPolygon; -import org.elasticsearch.geometry.Point; -import org.elasticsearch.geometry.Polygon; -import org.elasticsearch.geometry.Rectangle; import org.elasticsearch.geometry.ShapeType; -import java.util.Comparator; - /** * Like {@link ShapeType} but has specific * types for when the geometry is a {@link GeometryCollection} and @@ -44,28 +31,11 @@ */ public enum DimensionalShapeType { POINT, - MULTIPOINT, - LINESTRING, - MULTILINESTRING, - POLYGON, - MULTIPOLYGON, - GEOMETRYCOLLECTION_POINTS, // highest-dimensional shapes are Points - GEOMETRYCOLLECTION_LINES, // highest-dimensional shapes are Lines - GEOMETRYCOLLECTION_POLYGONS; // highest-dimensional shapes are Polygons - - public static Comparator COMPARATOR = Comparator.comparingInt(DimensionalShapeType::centroidDimension); + LINE, + POLYGON; private static DimensionalShapeType[] values = values(); - public static DimensionalShapeType max(DimensionalShapeType s1, DimensionalShapeType s2) { - if (s1 == null) { - return s2; - } else if (s2 == null) { - return s1; - } - return COMPARATOR.compare(s1, s2) >= 0 ? s1 : s2; - } - public static DimensionalShapeType fromOrdinalByte(byte ordinal) { return values[Byte.toUnsignedInt(ordinal)]; } @@ -77,104 +47,4 @@ public void writeTo(ByteBuffersDataOutput out) { public static DimensionalShapeType readFrom(ByteArrayDataInput in) { return fromOrdinalByte(in.readByte()); } - - public static DimensionalShapeType forGeometry(Geometry geometry) { - return geometry.visit(new GeometryVisitor<>() { - private DimensionalShapeType st = null; - - @Override - public DimensionalShapeType visit(Circle circle) { - throw new IllegalArgumentException("invalid shape type found [Circle] while computing dimensional shape type"); - } - - @Override - public DimensionalShapeType visit(Line line) { - st = DimensionalShapeType.max(st, DimensionalShapeType.LINESTRING); - return st; - } - - @Override - public DimensionalShapeType visit(LinearRing ring) { - throw new UnsupportedOperationException("should not visit LinearRing"); - } - - @Override - public DimensionalShapeType visit(MultiLine multiLine) { - st = DimensionalShapeType.max(st, DimensionalShapeType.MULTILINESTRING); - return st; - } - - @Override - public DimensionalShapeType visit(MultiPoint multiPoint) { - st = DimensionalShapeType.max(st, DimensionalShapeType.MULTIPOINT); - return st; - } - - @Override - public DimensionalShapeType visit(MultiPolygon multiPolygon) { - st = DimensionalShapeType.max(st, DimensionalShapeType.MULTIPOLYGON); - return st; - } - - @Override - public DimensionalShapeType visit(Point point) { - st = DimensionalShapeType.max(st, DimensionalShapeType.POINT); - return st; - } - - @Override - public DimensionalShapeType visit(Polygon polygon) { - st = DimensionalShapeType.max(st, DimensionalShapeType.POLYGON); - return st; - } - - @Override - public DimensionalShapeType visit(Rectangle rectangle) { - st = DimensionalShapeType.max(st, DimensionalShapeType.POLYGON); - return st; - } - - @Override - public DimensionalShapeType visit(GeometryCollection collection) { - for (Geometry shape : collection) { - shape.visit(this); - } - int dimension = st.centroidDimension(); - if (dimension == 0) { - return DimensionalShapeType.GEOMETRYCOLLECTION_POINTS; - } else if (dimension == 1) { - return DimensionalShapeType.GEOMETRYCOLLECTION_LINES; - } else { - return DimensionalShapeType.GEOMETRYCOLLECTION_POLYGONS; - } - } - }); - } - - /** - * The integer representation of the dimension for the specific - * dimensional shape type. This is to be used by the centroid - * calculation to determine whether to add a sub-shape's centroid - * to the overall shape calculation. - * - * @return 0 for points, 1 for lines, 2 for polygons - */ - private int centroidDimension() { - switch (this) { - case POINT: - case MULTIPOINT: - case GEOMETRYCOLLECTION_POINTS: - return 0; - case LINESTRING: - case MULTILINESTRING: - case GEOMETRYCOLLECTION_LINES: - return 1; - case POLYGON: - case MULTIPOLYGON: - case GEOMETRYCOLLECTION_POLYGONS: - return 2; - default: - throw new IllegalStateException("dimension calculation of DimensionalShapeType [" + this + "] is not supported"); - } - } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/GeoCentroidAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/GeoCentroidAggregator.java index 529195a4e861c..72e4146dd6b0d 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/GeoCentroidAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/GeoCentroidAggregator.java @@ -111,7 +111,7 @@ public void collect(int doc, long bucket) throws IOException { // update the sum for (int i = 0; i < valueCount; ++i) { MultiGeoValues.GeoValue value = values.nextValue(); - int compares = DimensionalShapeType.COMPARATOR.compare(shapeType, value.dimensionalShapeType()); + int compares = shapeType.compareTo(value.dimensionalShapeType()); if (compares < 0) { double coordinateWeight = value.weight(); compensatedSumLat.reset(coordinateWeight * value.lat(), 0.0); diff --git a/server/src/test/java/org/elasticsearch/common/geo/CentroidCalculatorTests.java b/server/src/test/java/org/elasticsearch/common/geo/CentroidCalculatorTests.java index e1d30b2b8e86a..5a5fc222debd5 100644 --- a/server/src/test/java/org/elasticsearch/common/geo/CentroidCalculatorTests.java +++ b/server/src/test/java/org/elasticsearch/common/geo/CentroidCalculatorTests.java @@ -20,20 +20,37 @@ import org.elasticsearch.geo.GeometryTestUtils; import org.elasticsearch.geometry.Geometry; +import org.elasticsearch.geometry.GeometryCollection; import org.elasticsearch.geometry.Line; import org.elasticsearch.geometry.LinearRing; +import org.elasticsearch.geometry.MultiLine; +import org.elasticsearch.geometry.MultiPoint; import org.elasticsearch.geometry.Point; import org.elasticsearch.geometry.Polygon; +import org.elasticsearch.geometry.ShapeType; import org.elasticsearch.test.ESTestCase; +import java.util.ArrayList; import java.util.Collections; import java.util.List; +import static org.elasticsearch.common.geo.DimensionalShapeType.LINE; +import static org.elasticsearch.common.geo.DimensionalShapeType.POINT; +import static org.elasticsearch.common.geo.DimensionalShapeType.POLYGON; import static org.hamcrest.Matchers.anyOf; import static org.hamcrest.Matchers.equalTo; public class CentroidCalculatorTests extends ESTestCase { - private static final double DELTA = 0.000001; + private static final double DELTA = 0.000000001; + + public void testPoint() { + Point point = GeometryTestUtils.randomPoint(false); + CentroidCalculator calculator = new CentroidCalculator(point); + assertThat(calculator.getX(), equalTo(GeoUtils.normalizeLon(point.getX()))); + assertThat(calculator.getY(), equalTo(GeoUtils.normalizeLat(point.getY()))); + assertThat(calculator.sumWeight(), equalTo(1.0)); + assertThat(calculator.getDimensionalShapeType(), equalTo(POINT)); + } public void testLine() { double[] y = new double[] { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 }; @@ -61,6 +78,44 @@ public void testLine() { assertEquals(5.5, calculator.getY(), DELTA); } + public void testMultiLine() { + MultiLine multiLine = GeometryTestUtils.randomMultiLine(false); + double sumLineX = 0; + double sumLineY = 0; + double sumLineWeight = 0; + for (Line line : multiLine) { + CentroidCalculator calculator = new CentroidCalculator(line); + sumLineX += calculator.compSumX.value(); + sumLineY += calculator.compSumY.value(); + sumLineWeight += calculator.compSumWeight.value(); + } + CentroidCalculator calculator = new CentroidCalculator(multiLine); + + assertEquals(sumLineX / sumLineWeight, calculator.getX(), DELTA); + assertEquals(sumLineY / sumLineWeight, calculator.getY(), DELTA); + assertEquals(sumLineWeight, calculator.sumWeight(), DELTA); + assertThat(calculator.getDimensionalShapeType(), equalTo(LINE)); + } + + public void testMultiPoint() { + MultiPoint multiPoint = GeometryTestUtils.randomMultiPoint(false); + double sumPointX = 0; + double sumPointY = 0; + double sumPointWeight = 0; + for (Point point : multiPoint) { + sumPointX += point.getX(); + sumPointY += point.getY(); + sumPointWeight += 1; + } + + CentroidCalculator calculator = new CentroidCalculator(multiPoint); + assertEquals(sumPointX / sumPointWeight, calculator.getX(), DELTA); + assertEquals(sumPointY / sumPointWeight, calculator.getY(), DELTA); + assertEquals(sumPointWeight, calculator.sumWeight(), DELTA); + assertThat(calculator.getDimensionalShapeType(), equalTo(POINT)); + + } + public void testRoundingErrorAndNormalization() { double lonA = GeometryTestUtils.randomLon(); double latA = GeometryTestUtils.randomLat(); @@ -113,21 +168,208 @@ public void testPolyonWithHole() { } } + public void testLineAsClosedPoint() { + double lon = GeometryTestUtils.randomLon(); + double lat = GeometryTestUtils.randomLat(); + CentroidCalculator calculator = new CentroidCalculator(new Line(new double[] {lon, lon}, new double[] { lat, lat})); + assertThat(calculator.getX(), equalTo(GeoUtils.normalizeLon(lon))); + assertThat(calculator.getY(), equalTo(GeoUtils.normalizeLat(lat))); + assertThat(calculator.sumWeight(), equalTo(1.0)); + } + + public void testPolygonAsLine() { + // create a line that traces itself as a polygon + Line sourceLine = GeometryTestUtils.randomLine(false); + double[] x = new double[2 * sourceLine.length() - 1]; + double[] y = new double[2 * sourceLine.length() - 1]; + int idx = 0; + for (int i = 0; i < sourceLine.length(); i++) { + x[idx] = sourceLine.getX(i); + y[idx] = sourceLine.getY(i); + idx += 1; + } + for (int i = sourceLine.length() - 2; i >= 0; i--) { + x[idx] = sourceLine.getX(i); + y[idx] = sourceLine.getY(i); + idx += 1; + } + + Line line = new Line(x, y); + CentroidCalculator lineCalculator = new CentroidCalculator(line); + + Polygon polygon = new Polygon(new LinearRing(x, y)); + CentroidCalculator calculator = new CentroidCalculator(polygon); + + // sometimes precision issues yield non-zero areas. must verify that area is close to zero + if (calculator.getDimensionalShapeType() == POLYGON) { + assertEquals(0.0, calculator.sumWeight(), 1e-10); + } else { + assertThat(calculator.getDimensionalShapeType(), equalTo(LINE)); + assertThat(calculator.getX(), equalTo(lineCalculator.getX())); + assertThat(calculator.getY(), equalTo(lineCalculator.getY())); + assertThat(calculator.sumWeight(), equalTo(lineCalculator.compSumWeight.value())); + } + } + public void testPolygonWithEqualSizedHole() { Polygon polyWithHole = new Polygon(new LinearRing(new double[]{-50, 50, 50, -50, -50}, new double[]{-50, -50, 50, 50, -50}), Collections.singletonList(new LinearRing(new double[]{-50, -50, 50, 50, -50}, new double[]{-50, 50, 50, -50, -50}))); CentroidCalculator calculator = new CentroidCalculator(polyWithHole); - assertThat(calculator.getX(), equalTo(Double.NaN)); - assertThat(calculator.getY(), equalTo(Double.NaN)); - assertThat(calculator.sumWeight(), equalTo(0.0)); + assertThat(calculator.getX(), equalTo(0.0)); + assertThat(calculator.getY(), equalTo(0.0)); + assertThat(calculator.sumWeight(), equalTo(400.0)); + assertThat(calculator.getDimensionalShapeType(), equalTo(LINE)); } - public void testLineAsClosedPoint() { - double lon = GeometryTestUtils.randomLon(); - double lat = GeometryTestUtils.randomLat(); - CentroidCalculator calculator = new CentroidCalculator(new Line(new double[] {lon, lon}, new double[] { lat, lat})); - assertThat(calculator.getX(), equalTo(Double.NaN)); - assertThat(calculator.getY(), equalTo(Double.NaN)); - assertThat(calculator.sumWeight(), equalTo(0.0)); + public void testPolygonAsPoint() { + Point point = GeometryTestUtils.randomPoint(false); + Polygon polygon = new Polygon(new LinearRing(new double[] { point.getX(), point.getX(), point.getX(), point.getX() }, + new double[] { point.getY(), point.getY(), point.getY(), point.getY() })); + CentroidCalculator calculator = new CentroidCalculator(polygon); + assertThat(calculator.getX(), equalTo(GeoUtils.normalizeLon(point.getX()))); + assertThat(calculator.getY(), equalTo(GeoUtils.normalizeLat(point.getY()))); + assertThat(calculator.sumWeight(), equalTo(1.0)); + assertThat(calculator.getDimensionalShapeType(), equalTo(POINT)); + } + + public void testGeometryCollection() { + int numPoints = randomIntBetween(0, 3); + int numLines = randomIntBetween(0, 3); + int numPolygons = randomIntBetween(0, 3); + + if (numPoints == 0 && numLines == 0 && numPolygons == 0) { + numPoints = 1; + numLines = 1; + numPolygons = 1; + } + List shapes = new ArrayList<>(); + for (int i = 0; i < numPoints; i++) { + if (randomBoolean()) { + shapes.add(GeometryTestUtils.randomPoint(false)); + } else { + shapes.add(GeometryTestUtils.randomMultiPoint(false)); + } + } + for (int i = 0; i < numLines; i++) { + if (randomBoolean()) { + shapes.add(GeometryTestUtils.randomLine(false)); + } else { + shapes.add(GeometryTestUtils.randomMultiLine(false)); + } + } + for (int i = 0; i < numPolygons; i++) { + if (randomBoolean()) { + shapes.add(GeometryTestUtils.randomPolygon(false)); + } else { + shapes.add(GeometryTestUtils.randomMultiPolygon(false)); + } + } + + DimensionalShapeType dimensionalShapeType = numPolygons > 0 ? POLYGON : numLines > 0 ? LINE : POINT; + + // addFromCalculator is only adding from shapes with the highest dimensionalShapeType + CentroidCalculator addFromCalculator = null; + for (Geometry shape : shapes) { + if ((shape.type() == ShapeType.MULTIPOLYGON || shape.type() == ShapeType.POLYGON) || + (dimensionalShapeType == LINE && (shape.type() == ShapeType.LINESTRING || shape.type() == ShapeType.MULTILINESTRING)) || + (dimensionalShapeType == POINT && (shape.type() == ShapeType.POINT || shape.type() == ShapeType.MULTIPOINT))) { + if (addFromCalculator == null) { + addFromCalculator = new CentroidCalculator(shape); + } else { + addFromCalculator.addFrom(new CentroidCalculator(shape)); + } + } + } + + // shuffle + if (randomBoolean()) { + Collections.shuffle(shapes, random()); + } else if (randomBoolean()) { + Collections.reverse(shapes); + } + + GeometryCollection collection = new GeometryCollection<>(shapes); + CentroidCalculator calculator = new CentroidCalculator(collection); + + assertThat(addFromCalculator.getDimensionalShapeType(), equalTo(dimensionalShapeType)); + assertThat(calculator.getDimensionalShapeType(), equalTo(dimensionalShapeType)); + assertEquals(calculator.getX(), addFromCalculator.getX(), DELTA); + assertEquals(calculator.getY(), addFromCalculator.getY(), DELTA); + assertEquals(calculator.sumWeight(), addFromCalculator.sumWeight(), DELTA); + } + + public void testAddFrom() { + Point point = GeometryTestUtils.randomPoint(false); + Line line = GeometryTestUtils.randomLine(false); + Polygon polygon = GeometryTestUtils.randomPolygon(false); + + // point add point + { + CentroidCalculator calculator = new CentroidCalculator(point); + calculator.addFrom(new CentroidCalculator(point)); + assertThat(calculator.compSumX.value(), equalTo(2 * point.getX())); + assertThat(calculator.compSumY.value(), equalTo(2 * point.getY())); + assertThat(calculator.sumWeight(), equalTo(2.0)); + } + + // point add line/polygon + { + CentroidCalculator lineCalculator = new CentroidCalculator(line); + CentroidCalculator calculator = new CentroidCalculator(point); + calculator.addFrom(lineCalculator); + assertThat(calculator.getX(), equalTo(lineCalculator.getX())); + assertThat(calculator.getY(), equalTo(lineCalculator.getY())); + assertThat(calculator.sumWeight(), equalTo(lineCalculator.sumWeight())); + } + + // line add point + { + CentroidCalculator lineCalculator = new CentroidCalculator(line); + CentroidCalculator calculator = new CentroidCalculator(line); + calculator.addFrom(new CentroidCalculator(point)); + assertThat(calculator.getX(), equalTo(lineCalculator.getX())); + assertThat(calculator.getY(), equalTo(lineCalculator.getY())); + assertThat(calculator.sumWeight(), equalTo(lineCalculator.sumWeight())); + } + + // line add line + { + CentroidCalculator lineCalculator = new CentroidCalculator(line); + CentroidCalculator calculator = new CentroidCalculator(line); + calculator.addFrom(lineCalculator); + assertEquals(2 * lineCalculator.compSumX.value(), calculator.compSumX.value(), DELTA); + assertEquals(2 * lineCalculator.compSumY.value(), calculator.compSumY.value(), DELTA); + assertEquals(2 * lineCalculator.sumWeight(), calculator.sumWeight(), DELTA); + } + + // line add polygon + { + CentroidCalculator polygonCalculator = new CentroidCalculator(polygon); + CentroidCalculator calculator = new CentroidCalculator(line); + calculator.addFrom(polygonCalculator); + assertThat(calculator.getX(), equalTo(polygonCalculator.getX())); + assertThat(calculator.getY(), equalTo(polygonCalculator.getY())); + assertThat(calculator.sumWeight(), equalTo(calculator.sumWeight())); + } + + // polygon add point/line + { + CentroidCalculator polygonCalculator = new CentroidCalculator(polygon); + CentroidCalculator calculator = new CentroidCalculator(polygon); + calculator.addFrom(new CentroidCalculator(randomBoolean() ? point : line)); + assertThat(calculator.getX(), equalTo(polygonCalculator.getX())); + assertThat(calculator.getY(), equalTo(polygonCalculator.getY())); + assertThat(calculator.sumWeight(), equalTo(calculator.sumWeight())); + } + + // polygon add polygon + { + CentroidCalculator polygonCalculator = new CentroidCalculator(polygon); + CentroidCalculator calculator = new CentroidCalculator(polygon); + calculator.addFrom(polygonCalculator); + assertThat(calculator.compSumX.value(), equalTo(2 * polygonCalculator.compSumX.value())); + assertThat(calculator.compSumY.value(), equalTo(2 * polygonCalculator.compSumY.value())); + assertThat(calculator.sumWeight(), equalTo(2 * polygonCalculator.sumWeight())); + } } } diff --git a/server/src/test/java/org/elasticsearch/common/geo/DimensionalShapeTypeTests.java b/server/src/test/java/org/elasticsearch/common/geo/DimensionalShapeTypeTests.java index 53297decbd83f..9966f87dc97e2 100644 --- a/server/src/test/java/org/elasticsearch/common/geo/DimensionalShapeTypeTests.java +++ b/server/src/test/java/org/elasticsearch/common/geo/DimensionalShapeTypeTests.java @@ -28,16 +28,10 @@ public class DimensionalShapeTypeTests extends ESTestCase { public void testValidOrdinals() { - assertThat(DimensionalShapeType.values().length, equalTo(9)); + assertThat(DimensionalShapeType.values().length, equalTo(3)); assertThat(DimensionalShapeType.POINT.ordinal(), equalTo(0)); - assertThat(DimensionalShapeType.MULTIPOINT.ordinal(), equalTo(1)); - assertThat(DimensionalShapeType.LINESTRING.ordinal(), equalTo(2)); - assertThat(DimensionalShapeType.MULTILINESTRING.ordinal(), equalTo(3)); - assertThat(DimensionalShapeType.POLYGON.ordinal(), equalTo(4)); - assertThat(DimensionalShapeType.MULTIPOLYGON.ordinal(), equalTo(5)); - assertThat(DimensionalShapeType.GEOMETRYCOLLECTION_POINTS.ordinal(), equalTo(6)); - assertThat(DimensionalShapeType.GEOMETRYCOLLECTION_LINES.ordinal(), equalTo(7)); - assertThat(DimensionalShapeType.GEOMETRYCOLLECTION_POLYGONS.ordinal(), equalTo(8)); + assertThat(DimensionalShapeType.LINE.ordinal(), equalTo(1)); + assertThat(DimensionalShapeType.POLYGON.ordinal(), equalTo(2)); } public void testSerialization() { diff --git a/server/src/test/java/org/elasticsearch/common/geo/TriangleTreeTests.java b/server/src/test/java/org/elasticsearch/common/geo/TriangleTreeTests.java index 71ac0a550c820..3de9d70d459ab 100644 --- a/server/src/test/java/org/elasticsearch/common/geo/TriangleTreeTests.java +++ b/server/src/test/java/org/elasticsearch/common/geo/TriangleTreeTests.java @@ -57,9 +57,9 @@ public class TriangleTreeTests extends ESTestCase { public void testDimensionalShapeType() throws IOException { GeoShapeIndexer indexer = new GeoShapeIndexer(true, "test"); assertDimensionalShapeType(randomPoint(false), DimensionalShapeType.POINT); - assertDimensionalShapeType(randomMultiPoint(false), DimensionalShapeType.MULTIPOINT); - assertDimensionalShapeType(randomLine(false), DimensionalShapeType.LINESTRING); - assertDimensionalShapeType(randomMultiLine(false), DimensionalShapeType.MULTILINESTRING); + assertDimensionalShapeType(randomMultiPoint(false), DimensionalShapeType.POINT); + assertDimensionalShapeType(randomLine(false), DimensionalShapeType.LINE); + assertDimensionalShapeType(randomMultiLine(false), DimensionalShapeType.LINE); Geometry randoPoly = indexer.prepareForIndexing(randomValueOtherThanMany(g -> { try { Geometry newGeo = indexer.prepareForIndexing(g); @@ -69,20 +69,20 @@ public void testDimensionalShapeType() throws IOException { } }, () -> randomPolygon(false))); assertDimensionalShapeType(randoPoly, DimensionalShapeType.POLYGON); - assertDimensionalShapeType(indexer.prepareForIndexing(randomMultiPolygon(false)), DimensionalShapeType.MULTIPOLYGON); + assertDimensionalShapeType(indexer.prepareForIndexing(randomMultiPolygon(false)), DimensionalShapeType.POLYGON); assertDimensionalShapeType(randomRectangle(), DimensionalShapeType.POLYGON); assertDimensionalShapeType(randomFrom( new GeometryCollection<>(List.of(randomPoint(false))), new GeometryCollection<>(List.of(randomMultiPoint(false))), new GeometryCollection<>(Collections.singletonList( new GeometryCollection<>(List.of(randomPoint(false), randomMultiPoint(false)))))) - , DimensionalShapeType.GEOMETRYCOLLECTION_POINTS); + , DimensionalShapeType.POINT); assertDimensionalShapeType(randomFrom( new GeometryCollection<>(List.of(randomPoint(false), randomLine(false))), new GeometryCollection<>(List.of(randomMultiPoint(false), randomMultiLine(false))), new GeometryCollection<>(Collections.singletonList( new GeometryCollection<>(List.of(randomPoint(false), randomLine(false)))))) - , DimensionalShapeType.GEOMETRYCOLLECTION_LINES); + , DimensionalShapeType.LINE); assertDimensionalShapeType(randomFrom( new GeometryCollection<>(List.of(randomPoint(false), indexer.prepareForIndexing(randomLine(false)), indexer.prepareForIndexing(randomPolygon(false)))), @@ -90,7 +90,7 @@ public void testDimensionalShapeType() throws IOException { new GeometryCollection<>(Collections.singletonList( new GeometryCollection<>(List.of(indexer.prepareForIndexing(randomLine(false)), indexer.prepareForIndexing(randomPolygon(false))))))) - , DimensionalShapeType.GEOMETRYCOLLECTION_POLYGONS); + , DimensionalShapeType.POLYGON); } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/GeoCentroidAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/GeoCentroidAggregatorTests.java index 892901fb9965d..780253919da16 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/GeoCentroidAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/GeoCentroidAggregatorTests.java @@ -194,7 +194,10 @@ public void testGeoShapeField() throws Exception { } catch (InvalidShapeException e) { // do not include geometry } - targetShapeType = DimensionalShapeType.max(targetShapeType, DimensionalShapeType.forGeometry(geometry)); + // find dimensional-shape-type of geometry + CentroidCalculator centroidCalculator = new CentroidCalculator(geometry); + DimensionalShapeType geometryShapeType = centroidCalculator.getDimensionalShapeType(); + targetShapeType = targetShapeType.compareTo(geometryShapeType) >= 0 ? targetShapeType : geometryShapeType; } try (Directory dir = newDirectory(); RandomIndexWriter w = new RandomIndexWriter(random(), dir)) { @@ -206,7 +209,7 @@ public void testGeoShapeField() throws Exception { CentroidCalculator calculator = new CentroidCalculator(geometry); document.add(new BinaryGeoShapeDocValuesField("field", GeoTestUtils.toDecodedTriangles(geometry), calculator)); w.addDocument(document); - if (DimensionalShapeType.COMPARATOR.compare(targetShapeType, calculator.getDimensionalShapeType()) == 0) { + if (targetShapeType.compareTo(calculator.getDimensionalShapeType()) == 0) { double weight = calculator.sumWeight(); compensatedSumLat.add(weight * calculator.getY()); compensatedSumLon.add(weight * calculator.getX()); From 3ad5ce73750bf724e186ec0e53c2794eaec52e88 Mon Sep 17 00:00:00 2001 From: Tal Levy Date: Tue, 18 Feb 2020 20:03:19 -0800 Subject: [PATCH 2/3] fix test bug --- .../aggregations/metrics/GeoCentroidAggregatorTests.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/GeoCentroidAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/GeoCentroidAggregatorTests.java index 780253919da16..57e56748d2f65 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/GeoCentroidAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/GeoCentroidAggregatorTests.java @@ -47,6 +47,8 @@ import java.util.List; import java.util.function.Function; +import static org.elasticsearch.common.geo.DimensionalShapeType.POINT; + public class GeoCentroidAggregatorTests extends AggregatorTestCase { private static final double GEOHASH_TOLERANCE = 1E-6D; @@ -176,7 +178,7 @@ public void testMultiValuedGeoPointField() throws Exception { public void testGeoShapeField() throws Exception { int numDocs = scaledRandomIntBetween(64, 256); List geometries = new ArrayList<>(); - DimensionalShapeType targetShapeType = null; + DimensionalShapeType targetShapeType = POINT; GeoShapeIndexer indexer = new GeoShapeIndexer(true, "test"); for (int i = 0; i < numDocs; i++) { Function geometryGenerator = ESTestCase.randomFrom( From 7779dfb5fffbe7d59d444661daa13cd1827e27d1 Mon Sep 17 00:00:00 2001 From: Tal Levy Date: Wed, 19 Feb 2020 07:46:54 -0800 Subject: [PATCH 3/3] remove gettings for compsums in centroidcalculator --- .../common/geo/CentroidCalculator.java | 25 +++++-------------- 1 file changed, 6 insertions(+), 19 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/geo/CentroidCalculator.java b/server/src/main/java/org/elasticsearch/common/geo/CentroidCalculator.java index 83ff5cad38d70..466384c849b2f 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/CentroidCalculator.java +++ b/server/src/main/java/org/elasticsearch/common/geo/CentroidCalculator.java @@ -46,7 +46,6 @@ public class CentroidCalculator { CompensatedSum compSumX; CompensatedSum compSumY; CompensatedSum compSumWeight; - private CentroidCalculatorVisitor visitor; private DimensionalShapeType dimensionalShapeType; @@ -60,18 +59,6 @@ public CentroidCalculator(Geometry geometry) { this.dimensionalShapeType = visitor.calculator.dimensionalShapeType; } - private CompensatedSum getCompSumX() { - return compSumX; - } - - private CompensatedSum getCompSumY() { - return compSumY; - } - - private CompensatedSum getCompSumWeight() { - return compSumWeight; - } - /** * adds a single coordinate to the running sum and count of coordinates * for centroid calculation @@ -111,9 +98,9 @@ public void addFrom(CentroidCalculator otherCalculator) { this.compSumWeight = otherCalculator.compSumWeight; } else if (compared == 0) { - this.compSumX.add(otherCalculator.getCompSumX().value()); - this.compSumY.add(otherCalculator.getCompSumY().value()); - this.compSumWeight.add(otherCalculator.getCompSumWeight().value()); + this.compSumX.add(otherCalculator.compSumX.value()); + this.compSumY.add(otherCalculator.compSumY.value()); + this.compSumWeight.add(otherCalculator.compSumWeight.value()); } // else (compared > 0) do not modify centroid calculation since otherCalculator is of lower dimension than this calculator } @@ -122,7 +109,7 @@ public void addFrom(CentroidCalculator otherCalculator) { */ public double getX() { // normalization required due to floating point precision errors - return GeoUtils.normalizeLon(getCompSumX().value() / getCompSumWeight().value()); + return GeoUtils.normalizeLon(compSumX.value() / compSumWeight.value()); } /** @@ -130,14 +117,14 @@ public double getX() { */ public double getY() { // normalization required due to floating point precision errors - return GeoUtils.normalizeLat(getCompSumY().value() / getCompSumWeight().value()); + return GeoUtils.normalizeLat(compSumY.value() / compSumWeight.value()); } /** * @return the sum of all the weighted coordinates summed in the calculator */ public double sumWeight() { - return getCompSumWeight().value(); + return compSumWeight.value(); } /**