You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2021/03/25 15:08:09 UTC
[GitHub] [lucene] jnystad opened a new pull request #41: LUCENE-9870: Fix Circle2D intersectsLine t-value (distance) range clamp
jnystad opened a new pull request #41:
URL: https://github.com/apache/lucene/pull/41
# Description
t-value (distance) should be between 0 and 1, not 0 and dotProduct.
Fixes missing matches when line magnitudeAB < 1.
Affects indexed line and polygon edges.
# Solution
Changed range check to be between 0 and 1. While clamping may be more correct, edge points are checked separately, so this shortcut should be valid.
# Tests
None.
# Checklist
Please review the following and check all that apply:
- [x] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/lucene/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [x] I have created a Jira issue and added the issue ID to my pull request title.
- [x] I have given Lucene maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [ ] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
Note: I have built 8.8.0 with this change to ensure proper behaviour in an elasticsearch 7.12 instance.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] iverase commented on pull request #41: LUCENE-9870: Fix Circle2D intersectsLine t-value (distance) range clamp
Posted by GitBox <gi...@apache.org>.
iverase commented on pull request #41:
URL: https://github.com/apache/lucene/pull/41#issuecomment-807022627
Thanks so much @jnystad,
Could I ask you to add a test that address the issue? (e.g a test that would fail before the fix) That's the way we would not hit the same failure twice :)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] iverase commented on a change in pull request #41: LUCENE-9870: Fix Circle2D intersectsLine t-value (distance) range clamp
Posted by GitBox <gi...@apache.org>.
iverase commented on a change in pull request #41:
URL: https://github.com/apache/lucene/pull/41#discussion_r602312995
##########
File path: lucene/core/src/test/org/apache/lucene/geo/TestCircle2D.java
##########
@@ -179,4 +179,28 @@ public void testRandomTriangles() {
}
}
}
+
+ public void testLineIntersects() {
+ Component2D circle2D;
+ if (random().nextBoolean()) {
+ Circle circle = new Circle(0, 0, 0.3f);
+ circle2D = LatLonGeometry.create(circle);
Review comment:
(LatLon) circle radius is defined in meters. I think it needs to be changed to ~35000 meters that is equivalent to .3 degrees at the equator?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] jnystad commented on pull request #41: LUCENE-9870: Fix Circle2D intersectsLine t-value (distance) range clamp
Posted by GitBox <gi...@apache.org>.
jnystad commented on pull request #41:
URL: https://github.com/apache/lucene/pull/41#issuecomment-808276711
I was probably just unlucky. Fixed now.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] jnystad commented on pull request #41: LUCENE-9870: Fix Circle2D intersectsLine t-value (distance) range clamp
Posted by GitBox <gi...@apache.org>.
jnystad commented on pull request #41:
URL: https://github.com/apache/lucene/pull/41#issuecomment-808249781
Will do. Could you please instruct how to control which test is run? I find the "randomness" to be a bit odd for a test suite. I ran several times without fail.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] iverase commented on pull request #41: LUCENE-9870: Fix Circle2D intersectsLine t-value (distance) range clamp
Posted by GitBox <gi...@apache.org>.
iverase commented on pull request #41:
URL: https://github.com/apache/lucene/pull/41#issuecomment-809191582
thanks @jnystad
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] iverase commented on pull request #41: LUCENE-9870: Fix Circle2D intersectsLine t-value (distance) range clamp
Posted by GitBox <gi...@apache.org>.
iverase commented on pull request #41:
URL: https://github.com/apache/lucene/pull/41#issuecomment-808240522
Thanks, I run the test and it seems to fail for the (LatLon)Circle. I added a comment with a possible fix. Could you add as well an entry in CHANGES.txt?It can be added to the Lucene 9.0 / bug section.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] iverase commented on pull request #41: LUCENE-9870: Fix Circle2D intersectsLine t-value (distance) range clamp
Posted by GitBox <gi...@apache.org>.
iverase commented on pull request #41:
URL: https://github.com/apache/lucene/pull/41#issuecomment-808259794
I don't think there is much. I normally add the following environmental variable the na was to repeat a test : `-Dtests.iters=50`. I run on your test and half of the test failed. Not sure why you didn't catch it, were you using the same seed?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] iverase merged pull request #41: LUCENE-9870: Fix Circle2D intersectsLine t-value (distance) range clamp
Posted by GitBox <gi...@apache.org>.
iverase merged pull request #41:
URL: https://github.com/apache/lucene/pull/41
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] jnystad commented on pull request #41: LUCENE-9870: Fix Circle2D intersectsLine t-value (distance) range clamp
Posted by GitBox <gi...@apache.org>.
jnystad commented on pull request #41:
URL: https://github.com/apache/lucene/pull/41#issuecomment-807205000
No problem. Test case added.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org