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
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:
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
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!
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
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
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
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.
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
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 =
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
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.
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
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
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
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
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.
--
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
18 matches
Mail list logo
