Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement weighted geo_shape centroid support #50297

Merged
merged 10 commits into from
Jan 8, 2020
Merged

Implement weighted geo_shape centroid support #50297

merged 10 commits into from
Jan 8, 2020

Conversation

talevy
Copy link
Contributor

@talevy talevy commented Dec 18, 2019 •
edited
Loading

This PR implements proper centroid calculations of geometries according to the
definition defined in #49887. To compute things correctly, an additional variable
encoded long representing the total weight for the centroid of the geometry in
a tree. This weight is always positive. Some tests are fixed, as they did not
have valid geometries.

Screen Shot 2019-12-18 at 7 51 13 PM

closes #49887.

@talevy talevy added WIP :Analytics/Geo Indexing, search aggregations of geo points and shapes labels Dec 18, 2019
@talevy talevy requested a review from iverase December 18, 2019 02:07
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (:Analytics/Geo)

@iverase
Copy link
Contributor

iverase commented Dec 18, 2019

I think the lat/lon values for the centroid need to be normalise to be in bounds. What I am wondering now is that we need to handle the dateline. I think the current calculation does not consider the dateline so two polygons at each side of the dateline might give a strange centroid?

On the other hand this has been always like that even for points so it might be just a feature.

@talevy talevy force-pushed the gdv-shapetype-centroid branch from 3a4a9f4 to 58b2243 Compare December 18, 2019 20:56
@talevy talevy marked this pull request as ready for review December 19, 2019 03:48
@talevy talevy removed the WIP label Dec 19, 2019
iverase
iverase requested changes Dec 19, 2019
View reviewed changes
Copy link
Contributor

@iverase iverase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Tal! I think the only issue at the moment is how we calculate weights for Polygons as we accept polygons with different orientations so we need to be careful there.

server/src/main/java/org/elasticsearch/common/geo/CentroidCalculator.java Show resolved Hide resolved
server/src/main/java/org/elasticsearch/search/aggregations/metrics/GeoCentroidAggregator.java Show resolved Hide resolved
server/src/test/java/org/elasticsearch/common/geo/CentroidCalculatorTests.java Show resolved Hide resolved
@talevy talevy requested review from iverase and nknize January 6, 2020 15:38
@talevy
Copy link
Contributor Author

talevy commented Jan 7, 2020

Heyo, I've updated to be agnostic to orientation with the help of Math.abs

talevy
talevy commented Jan 7, 2020
View reviewed changes
server/src/test/java/org/elasticsearch/common/geo/CentroidCalculatorTests.java
assertEquals(5.5, calculator.getY(), DELTA);
}

public void testRoundingErrorAndNormalization() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one issue I am not entirely sure how to best support. Due to how the centroid is calculated, it can happen that the longitude or latitude values are invalid and must be normalized.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ok. Normalization only happens when writing the centroid into the doc value.

@imotov imotov mentioned this pull request Jan 8, 2020
Closed
iverase
iverase approved these changes Jan 8, 2020
View reviewed changes
Copy link
Contributor

@iverase iverase left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm.

I am not sure about having to read the weight when reading the extent but I don't have a good suggestion. For the time being this looks good as I think we will have a last iteration to decide the final format of the doc value.

@talevy
Copy link
Contributor Author

talevy commented Jan 8, 2020

I am not sure about having to read the weight when reading the extent but I don't have a good suggestion

agreed. I will highlight this in our next review

@talevy talevy merged commit 2e15ea1 into elastic:geoshape-doc-values Jan 8, 2020
@talevy talevy deleted the gdv-shapetype-centroid branch January 8, 2020 16:34
@talevy talevy mentioned this pull request Feb 3, 2020
Closed
talevy added a commit that referenced this pull request Feb 10, 2020
@talevy
Implement weighted geo_shape centroid support (#50297)
2ec51be 
This PR implements proper centroid calculations of geometries according to the
definition defined in #49887. To compute things correctly, an additional variable
encoded long representing the total weight for the centroid of the geometry in
a tree. This weight is always positive. Some tests are fixed, as they did not
have valid geometries.

closes #49887.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iverase iverase iverase approved these changes

@nknize nknize Awaiting requested review from nknize

Assignees
No one assigned
Labels
:Analytics/Geo Indexing, search aggregations of geo points and shapes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@talevy @elasticmachine @iverase