Re: [PR] Fix a bug in ShapeTestUtil [lucene]
stefanvodita commented on PR #12287: URL: https://github.com/apache/lucene/pull/12287#issuecomment-1908668560 Thank you for persevering @heemin32! -- 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. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
Re: [PR] Fix a bug in ShapeTestUtil [lucene]
stefanvodita merged PR #12287: URL: https://github.com/apache/lucene/pull/12287 -- 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. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
Re: [PR] Fix a bug in ShapeTestUtil [lucene]
heemin32 commented on PR #12287: URL: https://github.com/apache/lucene/pull/12287#issuecomment-1908615744 Added CHANGELOG entry -- 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. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
Re: [PR] Fix a bug in ShapeTestUtil [lucene]
mikemccand commented on PR #12287: URL: https://github.com/apache/lucene/pull/12287#issuecomment-1908191816 > Maybe we can merge this fix as-is and continue the conversation about where test polygon generation should produce valid polygons in #12596. I +1. Progress not perfection! -- 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. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
Re: [PR] Fix a bug in ShapeTestUtil [lucene]
stefanvodita commented on PR #12287: URL: https://github.com/apache/lucene/pull/12287#issuecomment-1907818280 Maybe we can merge this fix as-is and continue the conversation about where test polygon generation should produce valid polygons in #12596. It's not clear to me now if there is anything to fix in #12596, but I think we agree that the fix in this PR should go ahead. @heemin32 - do you want to write a [CHANGES](https://github.com/apache/lucene/blob/main/lucene/CHANGES.txt) entry for Lucene 9.10? -- 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. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
Re: [PR] Fix a bug in ShapeTestUtil [lucene]
heemin32 commented on PR #12287: URL: https://github.com/apache/lucene/pull/12287#issuecomment-1907155750 Any update? -- 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. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
Re: [PR] Fix a bug in ShapeTestUtil [lucene]
github-actions[bot] commented on PR #12287: URL: https://github.com/apache/lucene/pull/12287#issuecomment-1907130242 This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the d...@lucene.apache.org list. Thank you for your contribution! -- 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. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
Re: [PR] Fix a bug in ShapeTestUtil [lucene]
heemin32 commented on PR #12287: URL: https://github.com/apache/lucene/pull/12287#issuecomment-1883551295 > Doing that could create a polygon that can't be tessellated. But `testLatLonPolygonCentroid()` does call `GeoTestUtils.nextPolygon()` directly and tessellation works fine. https://github.com/apache/lucene/blob/7b8aece125aabff2823626d5b939abf4747f63a7/lucene/core/src/test/org/apache/lucene/document/TestShapeDocValues.java#L77-L86 -- 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. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
Re: [PR] Fix a bug in ShapeTestUtil [lucene]
nknize commented on PR #12287: URL: https://github.com/apache/lucene/pull/12287#issuecomment-1883537742 > We need to call `ShapeTestUtil.nextPolygon();` directly from `testXYPolygonCentroid()`. Doing that could create a polygon that can't be tessellated. -- 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. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
Re: [PR] Fix a bug in ShapeTestUtil [lucene]
heemin32 commented on PR #12287: URL: https://github.com/apache/lucene/pull/12287#issuecomment-1883491755 Yes. But also what I meant is `testLatLonPolygonCentroid()` test calls `Polygon p = GeoTestUtil.nextPolygon();` directly, but `testXYPolygonCentroid()` test calls `XYPolygon p = (XYPolygon) BaseXYShapeTestCase.ShapeType.POLYGON.nextShape();` which calls `ShapeTestUtil.nextPolygon();` inside try-catch. We need to call `ShapeTestUtil.nextPolygon();` directly from `testXYPolygonCentroid()`. -- 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. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
Re: [PR] Fix a bug in ShapeTestUtil [lucene]
nknize commented on PR #12287: URL: https://github.com/apache/lucene/pull/12287#issuecomment-1883477836 > ... I believe `GeoTestUtil.nextPolygon()` returns valid polygon always but `ShapeTestUtil.nextPolygon()` does not. A quick glance looks like the difference is from [LUCENE-9192](https://github.com/apache/lucene/issues/10232) where ShapeTestUtil#nextPolygon will only consider executing the try-catch path on Nightly runs. -- 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. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
Re: [PR] Fix a bug in ShapeTestUtil [lucene]
heemin32 commented on PR #12287: URL: https://github.com/apache/lucene/pull/12287#issuecomment-1883449137 >That try-catch is intentional. However, the implementation between latlon and xy are different then. https://github.com/apache/lucene/blob/7b8aece125aabff2823626d5b939abf4747f63a7/lucene/core/src/test/org/apache/lucene/document/TestShapeDocValues.java#L77-L95 For latlon, it does not have try-catch statement wrapping `GeoTestUtil.nextPolygon()`. I believe `GeoTestUtil.nextPolygon()` returns valid polygon always but `ShapeTestUtil.nextPolygon()` does not. -- 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. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
Re: [PR] Fix a bug in ShapeTestUtil [lucene]
nknize commented on PR #12287: URL: https://github.com/apache/lucene/pull/12287#issuecomment-1883224420 > There are existing tests which should fails. https://github.com/apache/lucene/blob/main/lucene/core/src/test/org/apache/lucene/document/TestShapeDocValues.java#L67 > However because we are catching exception and try until we get the correct polygon, it never fails. That try-catch is intentional. The evil, esoteric / invalid test geometry generation is also intentional so we can maximize our test coverage across the tessellator and Shape indexing. This is why we have [Tessellator logic](https://github.com/apache/lucene/blob/4d916a754be608e7ffb4e5626e4f99b21b8513f1/lucene/core/src/java/org/apache/lucene/geo/Tessellator.java#L1293) to filter colinear and coplanar points so we can best effort index shapes that may not be well formed or may not have been pre-validated or cleaned. While we expect well formed data, that's not the real world. Users (especially geo users) often throw dirty data at the API. And to pre-validate geometries is an expensive operation. Not all users want to pay that price. This is why we left much of that diligence out of Lucene and in the hands of the integrating API. For example, see Elasticsearch [`ignore_malformed` parameter](https://github.com/elastic/elasticsearch/pull/24654). For the explicit shape doc value boundi ng box test referenced, we expect a valid polygon bounding box for test purposes, but (as just pointed out) that reused polygon generator could create a malformed one and an invalid bounding box. Rather than write a new polygon generator I opted to reuse the existing one and just retry until a valid polygon is created. What you could do is just write a new polygon generator that always creates a valid well-formed polygon. I'd be concerned, though, about new contributors always using that to create perfect data, which is not real world. -- 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. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
Re: [PR] Fix a bug in ShapeTestUtil [lucene]
stefanvodita commented on PR #12287: URL: https://github.com/apache/lucene/pull/12287#issuecomment-1883060775 We would still have a few more issues to solve after #12757, which are documented in #12596, but overall I agree that it makes sense to merge this as is. -- 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. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
Re: [PR] Fix a bug in ShapeTestUtil [lucene]
heemin32 commented on PR #12287: URL: https://github.com/apache/lucene/pull/12287#issuecomment-1881932572 There are existing tests which should fails. https://github.com/apache/lucene/blob/main/lucene/core/src/test/org/apache/lucene/document/TestShapeDocValues.java#L67 However because we are catching exception and try until we get the correct polygon, it never fails. https://github.com/apache/lucene/blob/main/lucene/core/src/test/org/apache/lucene/document/BaseXYShapeTestCase.java#L228-L235 I cannot remove the try-catch statement as of now because there are bug in `surpriseMePolygon()` and `createRegularPolygon()` which fails to create a valid polygon because of type casting from double to float. One of attempt to fix it is in https://github.com/apache/lucene/pull/12757. I would like to wait until the other issues are resolved and remove the try-catch statement rather than writing a unit test for just the test utility code. -- 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. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
Re: [PR] Fix a bug in ShapeTestUtil [lucene]
github-actions[bot] commented on PR #12287: URL: https://github.com/apache/lucene/pull/12287#issuecomment-1880903143 This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the d...@lucene.apache.org list. Thank you for your contribution! -- 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. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
Re: [PR] Fix a bug in ShapeTestUtil [lucene]
stefanvodita commented on PR #12287: URL: https://github.com/apache/lucene/pull/12287#issuecomment-1793516615 The test could call the modified methods with a random `box` and assert that all vertices of the given polygon are different. We can reuse `hasIdenticalVertices` from #12757. -- 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. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
Re: [PR] Fix a bug in ShapeTestUtil [lucene]
mikemccand commented on PR #12287: URL: https://github.com/apache/lucene/pull/12287#issuecomment-1790817479 Thanks @heemin32 for taking the effort to bring the fix down to Lucene, from OpenSearch test failures. A dedicated Lucene unit test would be great. Maybe @nknize could help evaluate the change? -- 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. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
