[GitHub] [lucene-solr] dsmiley commented on a change in pull request #709: LUCENE-8850: Calculate the area of a polygon and throw error when values are invalid
dsmiley commented on a change in pull request #709: LUCENE-8850: Calculate the area of a polygon and throw error when values are invalid URL: https://github.com/apache/lucene-solr/pull/709#discussion_r298616974 ## File path: lucene/core/src/java/org/apache/lucene/geo/Polygon.java ## @@ -96,34 +99,50 @@ public Polygon(double[] polyLats, double[] polyLons, Polygon... holes) { this.holes = holes.clone(); // compute bounding box -double minLat = polyLats[0]; -double maxLat = polyLats[0]; -double minLon = polyLons[0]; -double maxLon = polyLons[0]; +double minLat = Double.POSITIVE_INFINITY; +double maxLat = Double.NEGATIVE_INFINITY; +double minLon = Double.POSITIVE_INFINITY; +double maxLon = Double.NEGATIVE_INFINITY; double windingSum = 0d; final int numPts = polyLats.length - 1; -for (int i = 1, j = 0; i < numPts; j = i++) { +for (int i = 0; i < numPts; i++) { minLat = Math.min(polyLats[i], minLat); maxLat = Math.max(polyLats[i], maxLat); minLon = Math.min(polyLons[i], minLon); maxLon = Math.max(polyLons[i], maxLon); // compute signed area - windingSum += (polyLons[j] - polyLons[numPts])*(polyLats[i] - polyLats[numPts]) - - (polyLats[j] - polyLats[numPts])*(polyLons[i] - polyLons[numPts]); + windingSum += polyLons[i] * polyLats[i + 1] - polyLats[i] * polyLons[i + 1]; +} +if (windingSum == 0) { + throw new IllegalArgumentException("Cannot compute the polygon / hole orientation."); } this.minLat = minLat; this.maxLat = maxLat; this.minLon = minLon; this.maxLon = maxLon; this.windingOrder = (windingSum < 0) ? GeoUtils.WindingOrder.CCW : GeoUtils.WindingOrder.CW; +double area = Math.abs(windingSum / 2d); +for (Polygon hole : holes) { + area -= hole.area(); +} +if (area <= 0) { + throw new IllegalArgumentException("Polygon has an invalid area (area = " + area + ")."); +} +this.area = area; + } /** returns the number of vertex points */ public int numPoints() { return polyLats.length; } + /** returns the area of the polygon */ + public double area() { Review comment: Please add a comment to the code to to link to the "shoelace method" that Nick points at. I think it's important for non-trivial algorithms to incorporate references where the readers of this code can learn more. Who knows... maybe some "velcro method" will come along and be superior and then some student looking at this code will point this out to us :-) This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #709: LUCENE-8850: Calculate the area of a polygon and throw error when values are invalid
dsmiley commented on a change in pull request #709: LUCENE-8850: Calculate the area of a polygon and throw error when values are invalid URL: https://github.com/apache/lucene-solr/pull/709#discussion_r297927222 ## File path: lucene/core/src/java/org/apache/lucene/geo/Polygon.java ## @@ -96,34 +99,50 @@ public Polygon(double[] polyLats, double[] polyLons, Polygon... holes) { this.holes = holes.clone(); // compute bounding box -double minLat = polyLats[0]; -double maxLat = polyLats[0]; -double minLon = polyLons[0]; -double maxLon = polyLons[0]; +double minLat = Double.POSITIVE_INFINITY; +double maxLat = Double.NEGATIVE_INFINITY; +double minLon = Double.POSITIVE_INFINITY; +double maxLon = Double.NEGATIVE_INFINITY; double windingSum = 0d; final int numPts = polyLats.length - 1; -for (int i = 1, j = 0; i < numPts; j = i++) { +for (int i = 0; i < numPts; i++) { minLat = Math.min(polyLats[i], minLat); maxLat = Math.max(polyLats[i], maxLat); minLon = Math.min(polyLons[i], minLon); maxLon = Math.max(polyLons[i], maxLon); // compute signed area - windingSum += (polyLons[j] - polyLons[numPts])*(polyLats[i] - polyLats[numPts]) - - (polyLats[j] - polyLats[numPts])*(polyLons[i] - polyLons[numPts]); + windingSum += polyLons[i] * polyLats[i + 1] - polyLats[i] * polyLons[i + 1]; +} +if (windingSum == 0) { + throw new IllegalArgumentException("Cannot compute the polygon / hole orientation."); } this.minLat = minLat; this.maxLat = maxLat; this.minLon = minLon; this.maxLon = maxLon; this.windingOrder = (windingSum < 0) ? GeoUtils.WindingOrder.CCW : GeoUtils.WindingOrder.CW; +double area = Math.abs(windingSum / 2d); +for (Polygon hole : holes) { + area -= hole.area(); +} +if (area <= 0) { + throw new IllegalArgumentException("Polygon has an invalid area (area = " + area + ")."); +} +this.area = area; + } /** returns the number of vertex points */ public int numPoints() { return polyLats.length; } + /** returns the area of the polygon */ + public double area() { Review comment: I can imagine some asking what the units are. And why not getArea? Maybe this should be marked @lucene.internal or package local even if we don't want to expose it. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org For additional commands, e-mail: dev-h...@lucene.apache.org