fix tests for geo-centroid and revert to normalized lat/lon

talevy · talevy · commit 58b22431289a · 2019-12-18T12:56:29.000-08:00
diff --git a/server/src/main/java/org/elasticsearch/common/geo/CentroidCalculator.java b/server/src/main/java/org/elasticsearch/common/geo/CentroidCalculator.java
@@ -169,7 +169,7 @@ public Void visit(LinearRing ring) {
          */
         private void visitSignedRing(LinearRing ring, int sign) {
             for (int i = 0; i < ring.length() - 1; i++) {
-                double weight = ring.getX(i) * ring.getY(i + 1) + ring.getY(i) * ring.getX(i + 1);
+                double weight = ring.getX(i + 1) * ring.getY(i) - ring.getX(i) * ring.getY(i + 1);
                 calculator.addCoordinate((ring.getX(i) + ring.getX(i + 1)) / 2, (ring.getY(i) + ring.getY(i + 1)) / 2, sign * weight);
             }
         }
diff --git a/server/src/main/java/org/elasticsearch/common/geo/TriangleTreeReader.java b/server/src/main/java/org/elasticsearch/common/geo/TriangleTreeReader.java
@@ -33,7 +33,7 @@
  * relations against the serialized triangle tree.
  */
 public class TriangleTreeReader {
-    private static final int CENTROID_HEADER_SIZE_IN_BYTES = 25; // x-coord (4), y-coord (4), shape-type (1), sum-weight (8)
+    private static final int CENTROID_HEADER_SIZE_IN_BYTES = 17; // x-coord (4), y-coord (4), shape-type (1), sum-weight (8)
 
     private final ByteArrayDataInput input;
     private final CoordinateEncoder coordinateEncoder;
@@ -77,26 +77,26 @@ public Extent getExtent() {
     /**
      * returns the X coordinate of the centroid.
      */
-    public double getWeightedCentroidX() {
+    public double getUnweightedCentroidX() {
         input.setPosition(0);
-        return Double.longBitsToDouble(input.readLong());
+        return coordinateEncoder.decodeX(input.readInt());
     }
 
     /**
      * returns the Y coordinate of the centroid.
      */
-    public double getWeightedCentroidY() {
-        input.setPosition(8);
-        return Double.longBitsToDouble(input.readLong());
+    public double getUnweightedCentroidY() {
+        input.setPosition(4);
+        return coordinateEncoder.decodeY(input.readInt());
     }
 
     public DimensionalShapeType getDimensionalShapeType() {
-        input.setPosition(16);
+        input.setPosition(8);
         return DimensionalShapeType.readFrom(input);
     }
 
     public double getSumCentroidWeight() {
-        input.setPosition(17);
+        input.setPosition(9);
         return Double.longBitsToDouble(input.readLong());
     }
 
diff --git a/server/src/main/java/org/elasticsearch/common/geo/TriangleTreeWriter.java b/server/src/main/java/org/elasticsearch/common/geo/TriangleTreeWriter.java
@@ -47,8 +47,8 @@ public TriangleTreeWriter(List<ShapeField.DecodedTriangle> triangles, Coordinate
 
     /*** Serialize the interval tree in the provided data output */
     public void writeTo(ByteBuffersDataOutput out) throws IOException {
-        out.writeLong(Double.doubleToLongBits(centroidCalculator.getX()));
-        out.writeLong(Double.doubleToLongBits(centroidCalculator.getY()));
+        out.writeInt(coordinateEncoder.encodeX(centroidCalculator.getX()));
+        out.writeInt(coordinateEncoder.encodeY(centroidCalculator.getY()));
         centroidCalculator.getDimensionalShapeType().writeTo(out);
         out.writeLong(Double.doubleToLongBits(centroidCalculator.sumWeight()));
         out.writeInt(extent.top);
diff --git a/server/src/main/java/org/elasticsearch/index/fielddata/MultiGeoValues.java b/server/src/main/java/org/elasticsearch/index/fielddata/MultiGeoValues.java
@@ -185,15 +185,15 @@ public double weight() {
 
         @Override
         public double lat() {
-            return reader.getWeightedCentroidY();
+            return reader.getUnweightedCentroidY();
         }
 
         /**
          * @return the longitude of the centroid of the shape
          */
         @Override
         public double lon() {
-            return reader.getWeightedCentroidX();
+            return reader.getUnweightedCentroidX();
         }
 
         public static GeoShapeValue missing(String missing) {
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
@@ -88,6 +88,7 @@ public void collect(int doc, long bucket) throws IOException {
                 latCompensations = bigArrays.grow(latCompensations, bucket + 1);
                 weightCompensations = bigArrays.grow(weightCompensations, bucket + 1);
                 counts = bigArrays.grow(counts, bucket + 1);
+                dimensionalShapeTypes = bigArrays.grow(dimensionalShapeTypes, bucket + 1);
 
                 if (values.advanceExact(doc)) {
                     final int valueCount = values.docValueCount();
@@ -112,17 +113,19 @@ public void collect(int doc, long bucket) throws IOException {
                         MultiGeoValues.GeoValue value = values.nextValue();
                         int compares = DimensionalShapeType.COMPARATOR.compare(shapeType, value.dimensionalShapeType());
                         if (compares < 0) {
-                            compensatedSumLat.reset(value.lat(), 0.0);
-                            compensatedSumLon.reset(value.lon(), 0.0);
-                            compensatedSumWeight.reset(value.weight(), 0.0);
+                            double coordinateWeight = value.weight();
+                            compensatedSumLat.reset(coordinateWeight * value.lat(), 0.0);
+                            compensatedSumLon.reset(coordinateWeight * value.lon(), 0.0);
+                            compensatedSumWeight.reset(coordinateWeight, 0.0);
                             dimensionalShapeTypes.set(bucket, (byte) value.dimensionalShapeType().ordinal());
                         } else if (compares == 0) {
+                            double coordinateWeight = value.weight();
                             // weighted latitude
-                            compensatedSumLat.add(value.lat());
+                            compensatedSumLat.add(coordinateWeight * value.lat());
                             // weighted longitude
-                            compensatedSumLon.add(value.lon());
+                            compensatedSumLon.add(coordinateWeight * value.lon());
                             // weight
-                            compensatedSumWeight.add(value.weight());
+                            compensatedSumWeight.add(coordinateWeight);
                         }
 
                     }
diff --git a/server/src/test/java/org/elasticsearch/common/geo/CentroidCalculatorTests.java b/server/src/test/java/org/elasticsearch/common/geo/CentroidCalculatorTests.java
@@ -44,12 +44,12 @@ public void test() {
             System.arraycopy(y, 0, subY, 0, i + 1);
             Geometry geometry  = new Line(subX, subY);
             calculator = new CentroidCalculator(geometry);
-            assertThat(calculator.getX(), equalTo(xRunningAvg[i]));
-            assertThat(calculator.getY(), equalTo(yRunningAvg[i]));
+            assertEquals(xRunningAvg[i], calculator.getX(), 0.000001);
+            assertEquals(yRunningAvg[i], calculator.getY(), 0.000001);
         }
         CentroidCalculator otherCalculator = new CentroidCalculator(new Point(0, 0));
         calculator.addFrom(otherCalculator);
-        assertThat(calculator.getX(), equalTo(50.0));
-        assertThat(calculator.getY(), equalTo(5.0));
+        assertEquals(55.0, calculator.getX(), 0.0000001);
+        assertEquals(5.5, calculator.getY(), 0.0000001);
     }
 }
diff --git a/server/src/test/java/org/elasticsearch/common/geo/TriangleTreeTests.java b/server/src/test/java/org/elasticsearch/common/geo/TriangleTreeTests.java
@@ -98,8 +98,8 @@ public void testRectangleShape() throws IOException {
             // centroid is calculated using original double values but then loses precision as it is serialized as an integer
             int encodedCentroidX = GeoShapeCoordinateEncoder.INSTANCE.encodeX(((double) minX + maxX) / 2);
             int encodedCentroidY = GeoShapeCoordinateEncoder.INSTANCE.encodeY(((double) minY + maxY) / 2);
-            assertEquals(GeoShapeCoordinateEncoder.INSTANCE.decodeX(encodedCentroidX), reader.getWeightedCentroidX(), 0.0000001);
-            assertEquals(GeoShapeCoordinateEncoder.INSTANCE.decodeY(encodedCentroidY), reader.getWeightedCentroidY(), 0.0000001);
+            assertEquals(GeoShapeCoordinateEncoder.INSTANCE.decodeX(encodedCentroidX), reader.getUnweightedCentroidX(), 0.0000001);
+            assertEquals(GeoShapeCoordinateEncoder.INSTANCE.decodeY(encodedCentroidY), reader.getUnweightedCentroidY(), 0.0000001);
 
             // box-query touches bottom-left corner
             assertRelation(GeoRelation.QUERY_CROSSES, reader, getExtentFromBox(minX - randomIntBetween(1, 180 + minX),

Original file line number	Diff line number	Diff line change
`@@ -169,7 +169,7 @@ public Void visit(LinearRing ring) {`
`169`	`169`	`*/`
`170`	`170`	`private void visitSignedRing(LinearRing ring, int sign) {`
`171`	`171`	`for (int i = 0; i < ring.length() - 1; i++) {`
`172`		`- double weight = ring.getX(i) * ring.getY(i + 1) + ring.getY(i) * ring.getX(i + 1);`
	`172`	`+ double weight = ring.getX(i + 1) * ring.getY(i) - ring.getX(i) * ring.getY(i + 1);`
`173`	`173`	`calculator.addCoordinate((ring.getX(i) + ring.getX(i + 1)) / 2, (ring.getY(i) + ring.getY(i + 1)) / 2, sign * weight);`
`174`	`174`	`}`
`175`	`175`	`}`
Original file line number	Diff line number	Diff line change
`@@ -44,12 +44,12 @@ public void test() {`
`44`	`44`	`System.arraycopy(y, 0, subY, 0, i + 1);`
`45`	`45`	`Geometry geometry = new Line(subX, subY);`
`46`	`46`	`calculator = new CentroidCalculator(geometry);`
`47`		`- assertThat(calculator.getX(), equalTo(xRunningAvg[i]));`
`48`		`- assertThat(calculator.getY(), equalTo(yRunningAvg[i]));`
	`47`	`+ assertEquals(xRunningAvg[i], calculator.getX(), 0.000001);`
	`48`	`+ assertEquals(yRunningAvg[i], calculator.getY(), 0.000001);`
`49`	`49`	`}`
`50`	`50`	`CentroidCalculator otherCalculator = new CentroidCalculator(new Point(0, 0));`
`51`	`51`	`calculator.addFrom(otherCalculator);`
`52`		`- assertThat(calculator.getX(), equalTo(50.0));`
`53`		`- assertThat(calculator.getY(), equalTo(5.0));`
	`52`	`+ assertEquals(55.0, calculator.getX(), 0.0000001);`
	`53`	`+ assertEquals(5.5, calculator.getY(), 0.0000001);`
`54`	`54`	`}`
`55`	`55`	`}`