Lucene - Core
  1. Lucene - Core
  2. LUCENE-6544

Geo3d cleanup: Regularize path and polygon construction, plus consider adding ellipsoid surface distance method

    Details

    • Type: Improvement Improvement
    • 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

      Geo3d's way of constructing polygons and paths differs in that in one case you construct points and the other you feed lat/lon values directly to the builder. Probably both should be supported for both kinds of entity.

      Also it may be useful to have an accurate point-point ellipsoidal distance function. This is expensive and would be an addition to the arc distance we currently compute. It would probably be called "surface distance".

      1. LUCENE-6544.patch
        47 kB
        David Smiley
      2. LUCENE-6544.patch
        25 kB
        Karl Wright

        Activity

        Hide
        Karl Wright added a comment -

        First bit of cleanup; locally cache lat and lon in geopoints so we don't need to recompute it, and add an ellipsoid distance computation method.

        Show
        Karl Wright added a comment - First bit of cleanup; locally cache lat and lon in geopoints so we don't need to recompute it, and add an ellipsoid distance computation method.
        Hide
        Karl Wright added a comment -

        Finish the patch.

        Show
        Karl Wright added a comment - Finish the patch.
        Hide
        Karl Wright added a comment -

        David Smiley Is this a ticket you'd be willing to own?

        Show
        Karl Wright added a comment - David Smiley Is this a ticket you'd be willing to own?
        Hide
        David Smiley added a comment -

        Yes. Just give me a couple days to catch up with things I'm busy with.

        Show
        David Smiley added a comment - Yes. Just give me a couple days to catch up with things I'm busy with.
        Hide
        Karl Wright added a comment -

        I have another cleanup item I want to add to this: specifically, a fix so that a "circle" is in fact centered on the center point given. Hopefully adding this soon.

        Show
        Karl Wright added a comment - I have another cleanup item I want to add to this: specifically, a fix so that a "circle" is in fact centered on the center point given. Hopefully adding this soon.
        Hide
        Karl Wright added a comment -

        New patch including better math for "circles". Circles are now always centered on their center point, no matter how significantly ellipsoid the planet is.

        Show
        Karl Wright added a comment - New patch including better math for "circles". Circles are now always centered on their center point, no matter how significantly ellipsoid the planet is.
        Hide
        Karl Wright added a comment -

        David Smiley: I'm done with this now; anytime you are ready...

        Show
        Karl Wright added a comment - David Smiley : I'm done with this now; anytime you are ready...
        Hide
        David Smiley added a comment -

        I like the changes Karl.

        Some feedback:

        • GeoPath: I think the done() methods should throw an IllegalStateException if it is called after it has already been called (edgePoints is already defined)? And likewise, addPoint could throw if edgePoints is already defined.
        • PlanetModel: I think the surfaceDistance() method should include javadocs to mention that it's intended for non-spherical PlanetModel; for sphere use GeoPoint.arcDistance. And for that matter, GeoPoint.arcDistance could mention it's computation is surface of a sphere, and that for accurate ellipsoidal, use PlanetModel.surfaceDistance().

        The issue of thread-safety came to mind as I saw the lazy-evaluation of the latitude & longitude. There should be some docs/language somewhere so that people can understand that Geo3D generally isn't necessarily threadsafe, and GeoPoint in particular isn't. Lazy BBox calculation of Geo3dShape is another reason. This isn't necessarily a big deal but at least should be documented for these two classes, indicating how to construct them such that they are thread-safe. Come to think of it, even if an app knows to call getLatitude/Longitude on GeoPoint so that it's fully constructed, it has no way to do so on any use of GeoPoint embedded within other Geo3d shapes (such as GeoDegenerateHorizontalLine). What do you think?

        Show
        David Smiley added a comment - I like the changes Karl. Some feedback: GeoPath: I think the done() methods should throw an IllegalStateException if it is called after it has already been called (edgePoints is already defined)? And likewise, addPoint could throw if edgePoints is already defined. PlanetModel: I think the surfaceDistance() method should include javadocs to mention that it's intended for non-spherical PlanetModel; for sphere use GeoPoint.arcDistance. And for that matter, GeoPoint.arcDistance could mention it's computation is surface of a sphere, and that for accurate ellipsoidal, use PlanetModel.surfaceDistance(). The issue of thread-safety came to mind as I saw the lazy-evaluation of the latitude & longitude. There should be some docs/language somewhere so that people can understand that Geo3D generally isn't necessarily threadsafe, and GeoPoint in particular isn't. Lazy BBox calculation of Geo3dShape is another reason. This isn't necessarily a big deal but at least should be documented for these two classes, indicating how to construct them such that they are thread-safe. Come to think of it, even if an app knows to call getLatitude/Longitude on GeoPoint so that it's fully constructed, it has no way to do so on any use of GeoPoint embedded within other Geo3d shapes (such as GeoDegenerateHorizontalLine). What do you think?
        Hide
        Karl Wright added a comment -

        I'm fine with the GeoPath and PlanetModel suggestions.

        Thread safety is easier than you think, because all classes with lazy members are in fact immutable except for those lazy members, and they are based only other (immutable) characteristics of the classes in question. So the worst that can happen if more than one thread attempts to fill in one of these values is that it will be computed twice – one time unnecessarily. I suspect that therefore the only thing we really need to do is mention this in the javadoc for the lazy variable(s), and maybe (could be wrong about this) declare those variables to be volatile. Thoughts?

        Show
        Karl Wright added a comment - I'm fine with the GeoPath and PlanetModel suggestions. Thread safety is easier than you think, because all classes with lazy members are in fact immutable except for those lazy members, and they are based only other (immutable) characteristics of the classes in question. So the worst that can happen if more than one thread attempts to fill in one of these values is that it will be computed twice – one time unnecessarily. I suspect that therefore the only thing we really need to do is mention this in the javadoc for the lazy variable(s), and maybe (could be wrong about this) declare those variables to be volatile. Thoughts?
        Hide
        Karl Wright added a comment -

        Michael McCandless: Do I have the Java contract right? For a situation as described above, is "volatile" necessary?

        Show
        Karl Wright added a comment - Michael McCandless : Do I have the Java contract right? For a situation as described above, is "volatile" necessary?
        Hide
        Karl Wright added a comment -

        Actually, I think I answered my own question. According to this:

        http://stackoverflow.com/questions/4756536/what-operations-in-java-are-considered-atomic

        ... I need "volatile" to make updates to doubles or longs be atomic.

        Show
        Karl Wright added a comment - Actually, I think I answered my own question. According to this: http://stackoverflow.com/questions/4756536/what-operations-in-java-are-considered-atomic ... I need "volatile" to make updates to doubles or longs be atomic.
        Hide
        Karl Wright added a comment -

        David Smiley: this should meet all your needs...

        Show
        Karl Wright added a comment - David Smiley : this should meet all your needs...
        Hide
        David Smiley added a comment -

        FYI I did further edits and am now using ReviewBoard to share / collaborate:
        https://reviews.apache.org/r/35487/

        Show
        David Smiley added a comment - FYI I did further edits and am now using ReviewBoard to share / collaborate: https://reviews.apache.org/r/35487/
        Hide
        Karl Wright added a comment -

        Responded on reviewboard.

        Show
        Karl Wright added a comment - Responded on reviewboard.
        Hide
        David Smiley added a comment -

        Final patch is here, with CHANGES.txt as follows:

        * LUCENE-6544: Geo3D: (1) Regularize path & polygon construction, (2) add
          PlanetModel.surfaceDistance() (ellipsoidal calculation), (3) cache lat & lon
          in GeoPoint, (4) add thread-safety where missing -- Geo3dShape. (Karl Wright,
          David Smiley)
        

        I'll commit later at lunch.

        Show
        David Smiley added a comment - Final patch is here, with CHANGES.txt as follows: * LUCENE-6544: Geo3D: (1) Regularize path & polygon construction, (2) add PlanetModel.surfaceDistance() (ellipsoidal calculation), (3) cache lat & lon in GeoPoint, (4) add thread-safety where missing -- Geo3dShape. (Karl Wright, David Smiley) I'll commit later at lunch.
        Hide
        ASF subversion and git services added a comment -

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

        LUCENE-6544: Geo3D: Regularize path & polygon construction, add PlanetModel.surfaceDistance(), cache lat & lon in GeoPoint, add thread-safety where missing – Geo3dShape.

        Show
        ASF subversion and git services added a comment - Commit 1686285 from David Smiley in branch 'dev/trunk' [ https://svn.apache.org/r1686285 ] LUCENE-6544 : Geo3D: Regularize path & polygon construction, add PlanetModel.surfaceDistance(), cache lat & lon in GeoPoint, add thread-safety where missing – Geo3dShape.
        Hide
        ASF subversion and git services added a comment -

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

        LUCENE-6544: Geo3D: Regularize path & polygon construction, add PlanetModel.surfaceDistance(), cache lat & lon in GeoPoint, add thread-safety where missing – Geo3dShape.

        Show
        ASF subversion and git services added a comment - Commit 1686286 from David Smiley in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1686286 ] LUCENE-6544 : Geo3D: Regularize path & polygon construction, add PlanetModel.surfaceDistance(), cache lat & lon in GeoPoint, add thread-safety where missing – Geo3dShape.
        Hide
        David Smiley added a comment -

        Committed. Thanks Karl.

        Show
        David Smiley added a comment - Committed. Thanks Karl.
        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