Handle Elasticsearch GeoJSON circles using interpolation #67

warwickdufour · 2018-04-04T13:19:57Z

Proposed code to be able to render Elasticsearch GeoJSON circle geometry in GeoServer. The interpolation angular interval is varied according to the circumference of the circle (in ellipsoid surface units as opposed to pixels) and kept between upper and lower bounds.

An example output of NOTAMs over Cyprus is shown below. Note that the circles are displayed as 'sort-of' ellipses due to the latitude.

sjudeng

Thanks for the contribution. For consistency could you update to use four spaces for indent on all new lines?

sjudeng

Looks close. Some mostly minor feedback below. Thanks again for the contribution.

sjudeng · 2018-04-14T14:39:06Z

gt-elasticsearch/src/main/java/mil/nga/giat/data/elasticsearch/ElasticParserUtil.java

 static {
 GEO_HASH_PATTERN = Pattern.compile("[0123456789bcdefghjkmnpqrstuvwxyz]+");
 }

+ private static final Pattern ELASTIC_DISTANCE_PATTERN = Pattern.compile("([0-9]+(\\.[0-9]+)?)([a-zA-Z]+)");
+
+ private final GeodeticCalculator geodeticCalculator = new GeodeticCalculator(DefaultEllipsoid.WGS84);


Move geodeticCalculator initialization to constructor

sjudeng · 2018-04-14T14:39:24Z

gt-elasticsearch/src/main/java/mil/nga/giat/data/elasticsearch/ElasticParserUtil.java

+ case "CIRCLE": {
+ final List posList;
+ posList = (List) properties.get("coordinates");
+ String radius = (String) properties.get("radius");


Assume 'final' means that this is OK?

sjudeng · 2018-04-15T00:01:25Z

gt-elasticsearch/src/main/java/mil/nga/giat/data/elasticsearch/ElasticParserUtil.java

+ */
+ private Geometry createCircle(Coordinate centreCoord, String radius) {
+
+ String[] distanceAndUnitStrings = getDistanceAndUnitStrings(radius);


final here and below

sjudeng · 2018-04-15T00:02:00Z

gt-elasticsearch/src/main/java/mil/nga/giat/data/elasticsearch/ElasticParserUtil.java

+ private Geometry createCircle(Coordinate centreCoord, String radius) {
+
+ String[] distanceAndUnitStrings = getDistanceAndUnitStrings(radius);
+ double radM = FilterToElasticHelper.convertToMeters(Double.valueOf(distanceAndUnitStrings[0]), distanceAndUnitStrings[1]);


Split into two lines to keep line length under 120

Renamed variable to keep on one line

sjudeng · 2018-04-15T00:02:15Z

gt-elasticsearch/src/main/java/mil/nga/giat/data/elasticsearch/ElasticParserUtil.java

+ // Interpolate a circle on the surface of the ellipsoid such that the
+ // distance between interpolated point does not exceed (somewhat
+ // arbitrarily) 500m with the caveats that a circle must have a minimum
+ // of 40 points and a maximum of 500.


Ideally do you think the 500m and [40,500] values would be store parameters? If so would you create a separate issue to track that? Don't have to make the update here.

Agree that they should be 'store parameters' (not too sure what they are but I'm sure all will become clear if I put the effort in). Not sure if I should create a separate issue when the code I'm submitting has not been merged into the master branch?

sjudeng · 2018-04-15T00:04:01Z

gt-elasticsearch/src/main/java/mil/nga/giat/data/elasticsearch/ElasticParserUtil.java

+ */
+ private Geometry createCircle(Coordinate centreCoord, String radius) {
+
+ String[] distanceAndUnitStrings = getDistanceAndUnitStrings(radius);


If radius is invalid ({"",""} result or exception) geometry should be null?

Agree, done.

sjudeng · 2018-04-15T00:04:20Z

gt-elasticsearch/src/test/java/mil/nga/giat/data/elasticsearch/ElasticParserUtilTest.java

+ assertEquals("mm", distanceAndUnitStrings[1]);
+ distanceAndUnitStrings = ElasticParserUtil.getDistanceAndUnitStrings("999xyz");
+ assertEquals("999", distanceAndUnitStrings[0]);
+ assertEquals("xyz", distanceAndUnitStrings[1]);


Add test for no unit. Also add test for invalid value/unit.

sjudeng · 2018-04-15T00:04:44Z

gt-elasticsearch/src/main/java/mil/nga/giat/data/elasticsearch/FilterToElasticHelper.java

+ public static double convertToMeters(double distance, String unit) {
+ Double conversion = UNITS_MAP.get(unit);
+ if (conversion == null) {
+ conversion = new Double(1.0);


I think return of null would be better for invalid unit and it keeps it in line with previous behavior in toMeters.

On reflection I think I should revert to original to Meters() method and usage (and remove introduced methods) so functionality remains as you expect and I will create a private method in ElasticParserUtil for use when handling circles. Only change would be to make extended UNITS_MAP package private for reuse in ElasticParserUtil. Please ignore latest push, another soon to follow.

sjudeng · 2018-04-15T00:05:03Z

gt-elasticsearch/src/main/java/mil/nga/giat/data/elasticsearch/FilterToElasticHelper.java

 delegate.queryBuilder = ImmutableMap.of("bool", ImmutableMap.of("must_not", delegate.queryBuilder));
 }
 }

+ public static double convertToMeters(double distance, String unit) {


Add javadoc for public method

n/a as method being removed

sjudeng · 2018-04-15T00:08:15Z

gt-elasticsearch/src/main/java/mil/nga/giat/data/elasticsearch/ElasticParserUtil.java

 static {
 GEO_HASH_PATTERN = Pattern.compile("[0123456789bcdefghjkmnpqrstuvwxyz]+");
 }

+ private static final Pattern ELASTIC_DISTANCE_PATTERN = Pattern.compile("([0-9]+(\\.[0-9]+)?)([a-zA-Z]+)");


Docs suggest units are optional but pattern looks like it's required. Can you add a test for the missing/default unit case?

sjudeng

Thanks for the updates and additional tests. A few more minor comments below. The project follows the Sun Coding Conventions for consistency with GeoServer (see here) and the code style update requests below are based on those conventions.

sjudeng · 2018-04-19T21:56:49Z

gt-elasticsearch/src/main/java/mil/nga/giat/data/elasticsearch/ElasticParserUtil.java

+ private static final int MIN_CIRCLE_POINTS = 40;
+ private static final double MIN_CIRCLE_RADIUS_M = 0.001;
+
+


Remove extra line

sjudeng · 2018-04-19T21:57:30Z

gt-elasticsearch/src/main/java/mil/nga/giat/data/elasticsearch/ElasticParserUtil.java

+ case "CIRCLE": {
+ final List posList;
+ posList = (List) properties.get("coordinates");
+ String radius = (String) properties.get("radius");


Add final modifier (e.g. final String radius = ...).

sjudeng · 2018-04-19T22:00:17Z

gt-elasticsearch/src/main/java/mil/nga/giat/data/elasticsearch/ElasticParserUtil.java

+ return null;
+ }
+
+ double radM;


Use final modifier (final double radM). Same comment on all variable declarations in this code block.

sjudeng · 2018-04-19T22:01:22Z

gt-elasticsearch/src/main/java/mil/nga/giat/data/elasticsearch/ElasticParserUtil.java

+ }
+
+ // Reject circles with radii below an arbitrary minimum.
+ if(radM < MIN_CIRCLE_RADIUS_M) {


Add space (e.g. if (radM...) for consistency with style in file/project. Same comment on other if statements below.

sjudeng · 2018-04-19T22:03:13Z

gt-elasticsearch/src/main/java/mil/nga/giat/data/elasticsearch/ElasticParserUtil.java

+ throw new IllegalArgumentException();
+ }
+ Matcher matcher = ELASTIC_DISTANCE_PATTERN.matcher(distanceWithUnit);
+ double distance;


Can declare and initialize distance on one line below (e.g. final double distance = Double...). Same comment for unit.

sjudeng · 2018-04-19T22:22:46Z

gt-elasticsearch/src/main/java/mil/nga/giat/data/elasticsearch/ElasticParserUtil.java

+ if (unit != null && ! unit.isEmpty()) {
+ throw new IllegalArgumentException("Illegal unit: " + unit);
+ }
+ else {


One line } else {

sjudeng · 2018-04-19T22:24:56Z

gt-elasticsearch/src/main/java/mil/nga/giat/data/elasticsearch/ElasticParserUtil.java

+ }
+ return distance * conversion;
+ } else {
+ throw new IllegalArgumentException();


Add message

sjudeng · 2018-04-19T22:28:34Z

gt-elasticsearch/src/main/java/mil/nga/giat/data/elasticsearch/FilterToElasticHelper.java


 private static final long serialVersionUID = 1L;

 {
+ // Metric


Fix indentation on comment (here and for // Other below)

sjudeng · 2018-04-20T00:09:18Z

gt-elasticsearch/src/main/java/mil/nga/giat/data/elasticsearch/ElasticParserUtil.java

 public Geometry createGeometry(final Map<String, Object> properties) {
 final Geometry geometry;
 switch (String.valueOf(properties.get("type")).toUpperCase()) {
- case "POINT": {


Keep case statement aligned with switch

sjudeng

Looks great! Thanks for your patience on the review. Could you squash your commits for merge?

…interpolation with a great circle line radius.

warwickdufour · 2018-04-23T10:23:20Z

I think the squashing has worked OK but as it's the first time I have done it and there was an issue when pushing to 'origin' (had to redo and put a '+' in front of the branch name to get it to work) you might like to check.

sjudeng reviewed Apr 4, 2018

View reviewed changes

sjudeng suggested changes Apr 15, 2018

View reviewed changes

sjudeng reviewed Apr 20, 2018

View reviewed changes

sjudeng approved these changes Apr 20, 2018

View reviewed changes

Handle Elasticgeo Circles by approximating the circle boundary using …

ee83abd

…interpolation with a great circle line radius.

warwickdufour force-pushed the circles branch from d648b16 to ee83abd Compare April 23, 2018 09:45

sjudeng merged commit cf2d672 into ngageoint:master Apr 23, 2018

sjudeng mentioned this pull request Apr 23, 2018

Support for Elasticsearch circle geo_shape objects #64

Closed

		private static final int MIN_CIRCLE_POINTS = 40;
		private static final double MIN_CIRCLE_RADIUS_M = 0.001;

Handle Elasticsearch GeoJSON circles using interpolation #67

Handle Elasticsearch GeoJSON circles using interpolation #67

Conversation

warwickdufour commented Apr 4, 2018 • edited Loading

sjudeng left a comment

Choose a reason for hiding this comment

sjudeng 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

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

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

sjudeng 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjudeng left a comment

Choose a reason for hiding this comment

warwickdufour commented Apr 23, 2018

warwickdufour commented Apr 4, 2018 •

edited

Loading