Adds geo_centroid metric aggregator #13846
Conversation
nknize
added
review
:Analytics/Geo
|
@clintongormley Thanks for the documentation reminder. I'll finish that up. re: removal of #13433 I was thinking we make it optional (e.g., |
| pt[0] = pt[0] + (value.getLon() - pt[0]) / ++totalCounts; | ||
| pt[1] = pt[1] + (value.getLat() - pt[1]) / totalCounts; | ||
| } | ||
| centroids.set(bucket, XGeoUtils.mortonHash(pt[0], pt[1])); |
There was a problem hiding this comment.
should we not update the counts array with the new value of totalCounts here too?
There was a problem hiding this comment.
I find it weird that we serialise using XGeoUtils.mortonHash() but de-serialise using GeoUtils.fromIndexLong(). Could we change it so we serialise and de-serialise using the same object? That way it will be easier to see that changing the serialise method will affect the deserialise method
There was a problem hiding this comment.
@colings86 comments would have been useful eh? ¯_(ツ)_/¯ updating counts is done on line 90. I'll go ahead and rename some variables and add comments.
Re: serialization - we serialize using XGeoUtils.mortonHash() to obtain the encoded long, but then we deserialize with GeoPoint.fromIndexLong() which sets lat lon values for the GeoPoint object using the XGeoUtils.mortonUnhash... counterpart.
|
@nknize I left some comments but still want to try this out on some data too. In terms of this replacing the weighted centroid in the geohash-grid agg, I am a bit torn. On the one hand, I agree its is going to be a common use-case, but on the other hand I don't like having the same implementation in two different places as it adds a maintenance overhead. |
|
@colings86 Thanks for the feedback. I don't like having it in both places either. I opened #13912 to facilitate a discussion for whether we want to keep it "native" but make it optional in |
|
@colings86 removed centroid calculation from GeoHashGridAggregation. Centroid is a standalone metric aggregator. /cc @jpountz |
|
LGTM |
This commit adds a new metric aggregator for computing the geo_centroid over a set of geo_point fields. This can be combined with other aggregators (e.g., geohash_grid, significant_terms) for computing the geospatial centroid based on the document sets from other aggregation results.
This PR adds a new metric aggregator for computing the geo_centroid over a set of geo_point fields. This can be combined with other aggregators (e.g., geohash_grid, significant_terms) for computing the geospatial centroid based on the document sets from other aggregation results.
closes #13621