Lucene - Core
  1. Lucene - Core
  2. LUCENE-6597

Geo3d circle creation that covers whole globe throws an IllegalArgumentException

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.3
    • Component/s: modules/spatial
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      The following GeoCircle construction:

      new GeoCircle(PlanetModel.SPHERE, -20.0 * RADIANS_PER_DEGREE, -20.0 * RADIANS_PER_DEGREE, Math.PI);
      

      ... fails as follows:

      Degenerate/parallel vector constructed
      

      The reason is that the plane normal vector cannot be computed in that case. A special case is warranted for circles that cover the whole globe.

        Activity

        Hide
        Karl Wright added a comment -

        Patch for this issue.

        Show
        Karl Wright added a comment - Patch for this issue.
        Hide
        Karl Wright added a comment -

        David Smiley: Patch looks like it solves the IllegalArgumentException in the place I was seeing it, and doesn't break anything, so other than adding an explicit test I am good with it.

        Show
        Karl Wright added a comment - David Smiley : Patch looks like it solves the IllegalArgumentException in the place I was seeing it, and doesn't break anything, so other than adding an explicit test I am good with it.
        Hide
        David Smiley added a comment -

        I just took a quick look.

        Why does the intersects() method return false when the circle is the world; shouldn't it return true? Or maybe I'm misunderstanding the semantics.

        Do you think it may be worth constructing a special Circle subclass or special Plane subclass instead, thereby reducing the conditions you added to the methods? It's certainly no big deal to me; I have no strong opinion; I mostly wonder what you think. Given how you handled the numerous special cases of rectangles that are adjacent to poles & what'not as sub-classes, I'm surprised you didn't take a similar type of approach here.

        Can you add a test please? Perhaps make the random test sometimes in a rare while pick a world circle.

        Show
        David Smiley added a comment - I just took a quick look. Why does the intersects() method return false when the circle is the world; shouldn't it return true? Or maybe I'm misunderstanding the semantics. Do you think it may be worth constructing a special Circle subclass or special Plane subclass instead, thereby reducing the conditions you added to the methods? It's certainly no big deal to me; I have no strong opinion; I mostly wonder what you think. Given how you handled the numerous special cases of rectangles that are adjacent to poles & what'not as sub-classes, I'm surprised you didn't take a similar type of approach here. Can you add a test please? Perhaps make the random test sometimes in a rare while pick a world circle.
        Hide
        Karl Wright added a comment - - edited

        Why does the intersects() method return false when the circle is the world; shouldn't it return true? Or maybe I'm misunderstanding the semantics.

        You're misunderstanding the semantics. "intersects()" is whether the boundary intersects. There is no boundary, hence no intersection.

        Do you think it may be worth constructing a special Circle subclass or special Plane subclass instead, thereby reducing the conditions you added to the methods?

        I did think of that. If you introduced a GeoCircleFactory class, which you used to construct circles, that might make sense. Not sure this is worth it however. It really is only a single minor special case for a circle. The GeoBBox case is obviously rather different, because there really are a myriad of 3d shapes described by a bounding-box-style rectangle, each of which has significantly different logic.
        In the longer run, there are some shapes that get constructed via a factory, and others via a constructor. Regularizing usage to always rely on factories may make for a prettier API. But I still don't think I'd change the GeoCircle implementation.

        Can you add a test please? Perhaps make the random test sometimes in a rare while pick a world circle.

        It will. Before it would have gotten an illegal argument exception, though, and continued. But I will add an explicit test case.

        Show
        Karl Wright added a comment - - edited Why does the intersects() method return false when the circle is the world; shouldn't it return true? Or maybe I'm misunderstanding the semantics. You're misunderstanding the semantics. "intersects()" is whether the boundary intersects. There is no boundary, hence no intersection. Do you think it may be worth constructing a special Circle subclass or special Plane subclass instead, thereby reducing the conditions you added to the methods? I did think of that. If you introduced a GeoCircleFactory class, which you used to construct circles, that might make sense. Not sure this is worth it however. It really is only a single minor special case for a circle. The GeoBBox case is obviously rather different, because there really are a myriad of 3d shapes described by a bounding-box-style rectangle, each of which has significantly different logic. In the longer run, there are some shapes that get constructed via a factory, and others via a constructor. Regularizing usage to always rely on factories may make for a prettier API. But I still don't think I'd change the GeoCircle implementation. Can you add a test please? Perhaps make the random test sometimes in a rare while pick a world circle. It will. Before it would have gotten an illegal argument exception, though, and continued. But I will add an explicit test case.
        Hide
        Karl Wright added a comment -

        Test insures that full-world circles can be constructed, and basically function. Randomized tests can therefore exhaustively exercise relationships etc.

        Show
        Karl Wright added a comment - Test insures that full-world circles can be constructed, and basically function. Randomized tests can therefore exhaustively exercise relationships etc.
        Hide
        David Smiley added a comment -

        Test looks good.

        • What do you think about adding support for zero radius? Might as well; right? I started to do this to see what happens. First I needed to relax the arg check in the GeoCircle constructor. But then I found the SidedPlane.constructNormalizedPerpendicularSidedPlane returned null.
        • I noticed GeoCircle computes the cosign of the cutoffAngle but doesn't use it.
        • The IllegalArgumentException loop in Geo3dShapeRectRelationTestCase.testGeoCircleRect can now be removed, it appears.
        Show
        David Smiley added a comment - Test looks good. What do you think about adding support for zero radius? Might as well; right? I started to do this to see what happens. First I needed to relax the arg check in the GeoCircle constructor. But then I found the SidedPlane.constructNormalizedPerpendicularSidedPlane returned null. I noticed GeoCircle computes the cosign of the cutoffAngle but doesn't use it. The IllegalArgumentException loop in Geo3dShapeRectRelationTestCase.testGeoCircleRect can now be removed, it appears.
        Hide
        Karl Wright added a comment -

        What do you think about adding support for zero radius? Might as well; right? I started to do this to see what happens. First I needed to relax the arg check in the GeoCircle constructor. But then I found the SidedPlane.constructNormalizedPerpendicularSidedPlane returned null.

        Yes, exactly.

        While the whole-globe degenerate case is in fact useful and might be realistically encountered, and used to work before the WGS84 implementation, I've basically shied away from the none-of-the-globe or single-point side of things. GeoPath, GeoConvexPolygon, and GeoCircle all have potential single point solutions. I did add a degenerate point object for GeoBBoxes but only because I did not trust spatial4j to not generate such a thing algorithmically. But otherwise I would have resisted.

        If you believe it is critical to address this omission, you can either add a "tangent plane to a point" sided plane constructor, or a boolean that would be true for the special case. But, as I said, many other shapes would then probably need to be looked at too. Really a lot of work for zero benefit.

        I noticed GeoCircle computes the cosign of the cutoffAngle but doesn't use it.

        If unused then we shouldn't compute it. Feel free to get rid of it, or if you prefer, I will.

        The IllegalArgumentException loop in Geo3dShapeRectRelationTestCase.testGeoCircleRect can now be removed, it appears.

        Only if you take the correct steps to allow degenerate single-point circles. Just accepting a null circlePlane is not that step, since that currently would represent a whole-world circle, not a zero-world circle.

        Show
        Karl Wright added a comment - What do you think about adding support for zero radius? Might as well; right? I started to do this to see what happens. First I needed to relax the arg check in the GeoCircle constructor. But then I found the SidedPlane.constructNormalizedPerpendicularSidedPlane returned null. Yes, exactly. While the whole-globe degenerate case is in fact useful and might be realistically encountered, and used to work before the WGS84 implementation, I've basically shied away from the none-of-the-globe or single-point side of things. GeoPath, GeoConvexPolygon, and GeoCircle all have potential single point solutions. I did add a degenerate point object for GeoBBoxes but only because I did not trust spatial4j to not generate such a thing algorithmically. But otherwise I would have resisted. If you believe it is critical to address this omission, you can either add a "tangent plane to a point" sided plane constructor, or a boolean that would be true for the special case. But, as I said, many other shapes would then probably need to be looked at too. Really a lot of work for zero benefit. I noticed GeoCircle computes the cosign of the cutoffAngle but doesn't use it. If unused then we shouldn't compute it. Feel free to get rid of it, or if you prefer, I will. The IllegalArgumentException loop in Geo3dShapeRectRelationTestCase.testGeoCircleRect can now be removed, it appears. Only if you take the correct steps to allow degenerate single-point circles. Just accepting a null circlePlane is not that step, since that currently would represent a whole-world circle, not a zero-world circle.
        Hide
        Karl Wright added a comment -

        Also, David, this particular patch is not rocket science IMHO. It just corrects a case I broke with WGS84, which a test suite a work caught. If you want to extend the functionality, really we should be talking about another ticket.

        Show
        Karl Wright added a comment - Also, David, this particular patch is not rocket science IMHO. It just corrects a case I broke with WGS84, which a test suite a work caught. If you want to extend the functionality, really we should be talking about another ticket.
        Hide
        ASF subversion and git services added a comment -

        Commit 1687499 from David Smiley in branch 'dev/trunk'
        [ https://svn.apache.org/r1687499 ]

        LUCENE-6597: Geo3D's GeoCircle now supports a world-globe diameter.

        Show
        ASF subversion and git services added a comment - Commit 1687499 from David Smiley in branch 'dev/trunk' [ https://svn.apache.org/r1687499 ] LUCENE-6597 : Geo3D's GeoCircle now supports a world-globe diameter.
        Hide
        ASF subversion and git services added a comment -

        Commit 1687502 from David Smiley in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1687502 ]

        LUCENE-6597: Geo3D's GeoCircle now supports a world-globe diameter.

        Show
        ASF subversion and git services added a comment - Commit 1687502 from David Smiley in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1687502 ] LUCENE-6597 : Geo3D's GeoCircle now supports a world-globe diameter.
        Hide
        David Smiley added a comment -

        Thanks Karl.

        0-diameter circle was definitely not needed; was just seeing if it'd be particularly easy.

        Show
        David Smiley added a comment - Thanks Karl. 0-diameter circle was definitely not needed; was just seeing if it'd be particularly easy.
        Hide
        Shalin Shekhar Mangar added a comment -

        Bulk close for 5.3.0 release

        Show
        Shalin Shekhar Mangar added a comment - Bulk close for 5.3.0 release

          People

          • Assignee:
            David Smiley
            Reporter:
            Karl Wright
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development