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.

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

surpriseMePolygon and createRegularPolygon in test util class returns invalid polygon #12596

Open
heemin32 opened this issue Sep 26, 2023 · 2 comments
Labels

Comments

@heemin32
Copy link
Contributor

Description

https://github.com/apache/lucene/blob/main/lucene/test-framework/src/java/org/apache/lucene/tests/geo/ShapeTestUtil.java

surpriseMePolygon and createRegularPolygon returns invalid polygon where three or more points having same value due to type casting from double to float.

Version and environment details

No response

@stefanvodita
Copy link
Contributor

What happens with createRegularPolygon is very interesting. The smaller the polygon's radius is and the more vertices it has, the smaller the sides are. At some point they get so small, they can't fit on a float. I opened a PR which should fix the issue.

For surpriseMePolygon, the problem appears when we have a center with large coordinates and a small radius. The radius is too small to register when doing arithmetic with the center's coordinates. I added a retry mechanism.

There are other problems that I have to think about more, maybe in a separate PR. For example, what about large coordinates for createRegularPolygon? What about the case when the centers are at FLOAT_MAX? Then the radius becomes 0.

@stefanvodita
Copy link
Contributor

Another edge case that can cause problems is that of multiple points on the same line (e.g. 3 points in a row with the same x coordinate). This happens for regular and for "surprise" polygons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants