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: issues-unsubscr...@lucen
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 commen
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 any
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 uns
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 contributi
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 me
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 = (XYPoly
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](
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/7b8aece125aabff2823626d5b939abf4747f63
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
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 because
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 contributi
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 evaluate
18 matches
Mail list logo
