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

      WGS84 compatibility has been requested for geo3d. This involves working with an ellipsoid rather than a unit sphere. The general formula for an ellipsoid is:

      x^2/a^2 + y^2/b^2 + z^2/c^2 = 1

      1. LUCENE-6487.patch
        252 kB
        Karl Wright
      2. LUCENE-6487.patch
        251 kB
        Karl Wright
      3. LUCENE-6487.patch
        256 kB
        David Smiley
      4. LUCENE-6487.patch
        249 kB
        Karl Wright

        Activity

        Hide
        Karl Wright added a comment - - edited

        What I propose to do is introduce a planet model, as follows:

        public class PlanetModel {
          
          // Surface of the planet:
          // x^2/a^2 + y^2/b^2 + z^2/c^2 = 1.0
          // Scaling factors are a,b,c.
          public final double inverseA;
          public final double inverseB;
          public final double inverseC;
          public final double inverseASquared;
          public final double inverseBSquared;
          public final double inverseCSquared;
          // We do NOT include radius, because all computations in geo3d are in radians, not meters.
          
          /** Planet model corresponding to sphere. */
          public static final PlanetModel SPHERE = new PlanetModel(1.0,1.0,1.0);
          /** Planet model corresponding to WGS84 */
          public final static double WGS84_FLATTENING = 1.0/298.257223563;
          // For WGS84, the flattening is applied how?  Given we still want to scale by mean earth radius in meters, what gets scaled
          // and how?  Ask Nicholas.
          public static final PlanetModel WGS84 = new PlanetModel(1.0 - WGS84_FLATTENING, 1.0 - WGS84_FLATTENING, 1.0);
          
          public PlanetModel(final double a, final double b, final double c) {
            this.inverseA = 1.0 / a;
            this.inverseB = 1.0 / b;
            this.inverseC = 1.0 / c;
            this.inverseASquared = inverseA * inverseA;
            this.inverseBSquared = inverseB * inverseB;
            this.inverseCSquared = inverseC * inverseC;
          }
          
        }
        

        [~nknize@gmail.com], is this correct

        Show
        Karl Wright added a comment - - edited What I propose to do is introduce a planet model, as follows: public class PlanetModel { // Surface of the planet: // x^2/a^2 + y^2/b^2 + z^2/c^2 = 1.0 // Scaling factors are a,b,c. public final double inverseA; public final double inverseB; public final double inverseC; public final double inverseASquared; public final double inverseBSquared; public final double inverseCSquared; // We do NOT include radius, because all computations in geo3d are in radians, not meters. /** Planet model corresponding to sphere. */ public static final PlanetModel SPHERE = new PlanetModel(1.0,1.0,1.0); /** Planet model corresponding to WGS84 */ public final static double WGS84_FLATTENING = 1.0/298.257223563; // For WGS84, the flattening is applied how? Given we still want to scale by mean earth radius in meters, what gets scaled // and how? Ask Nicholas. public static final PlanetModel WGS84 = new PlanetModel(1.0 - WGS84_FLATTENING, 1.0 - WGS84_FLATTENING, 1.0); public PlanetModel( final double a, final double b, final double c) { this .inverseA = 1.0 / a; this .inverseB = 1.0 / b; this .inverseC = 1.0 / c; this .inverseASquared = inverseA * inverseA; this .inverseBSquared = inverseB * inverseB; this .inverseCSquared = inverseC * inverseC; } } [~nknize@gmail.com] , is this correct
        Hide
        Karl Wright added a comment -

        I'm making good progress on this work. The only code remaining that needs functional updating is the bounds computation, which will probably take a couple of days. I also need some help from Nicholas making sure that the ellipsoid coefficients are in fact correct for WGS84, and writing some WGS-specific tests.

        I'm attaching a preliminary patch, which contains all the structural changes necessary, plus everything other than the code listed above. It is for reference/review purposes only and should not yet be committed.

        Show
        Karl Wright added a comment - I'm making good progress on this work. The only code remaining that needs functional updating is the bounds computation, which will probably take a couple of days. I also need some help from Nicholas making sure that the ellipsoid coefficients are in fact correct for WGS84, and writing some WGS-specific tests. I'm attaching a preliminary patch, which contains all the structural changes necessary, plus everything other than the code listed above. It is for reference/review purposes only and should not yet be committed.
        Hide
        Karl Wright added a comment -

        Updating the patch with "completed" functionality, although incompletely tested and with a known problem. Specifically, paths are specified with a half-width for the straight segments, and that's been specified as an angle to date. That's fine, but with an ellipsoid, a pair of planes describing the corridor in that way might well fail to line up with the circles at the waypoints. We really need to determine the bounding planes in a different manner to be consistent. Something I will need to work out.

        Show
        Karl Wright added a comment - Updating the patch with "completed" functionality, although incompletely tested and with a known problem. Specifically, paths are specified with a half-width for the straight segments, and that's been specified as an angle to date. That's fine, but with an ellipsoid, a pair of planes describing the corridor in that way might well fail to line up with the circles at the waypoints. We really need to determine the bounding planes in a different manner to be consistent. Something I will need to work out.
        Hide
        Karl Wright added a comment -

        Updated patch, which deals properly with some ellipsoid issues around "circles" and paths. So now once again there are no known functional problems and we're back to adding tests.

        Still need to come up with the "proper" WGS84 ellipsoid; I think we want the scaling factors such that if you multiply all points by the mean earth radius you get the proper points in meters.

        Show
        Karl Wright added a comment - Updated patch, which deals properly with some ellipsoid issues around "circles" and paths. So now once again there are no known functional problems and we're back to adding tests. Still need to come up with the "proper" WGS84 ellipsoid; I think we want the scaling factors such that if you multiply all points by the mean earth radius you get the proper points in meters.
        Hide
        Karl Wright added a comment -

        Fix arc distance computation and geopoint computation.

        Show
        Karl Wright added a comment - Fix arc distance computation and geopoint computation.
        Hide
        Karl Wright added a comment -

        Add more tests

        Show
        Karl Wright added a comment - Add more tests
        Hide
        Karl Wright added a comment - - edited

        Hmm.

        Trying to introduce a random test for WGS84 using the same infrastructure as for a SPHERE, and getting the following:

           [junit4]   1> bbox=GeoSouthLatitudeZone: {toplat=-1.5024863073176973(-86.08612418550003)}
           [junit4]   1> spatialRect=Rect(minX=82.0,maxX=92.0,minY=-86.0,maxY=-72.0), circleShape=Geo3dShape{planetmodel=PlanetModel: {a=1.0011188180710464, b=1.0011188180710464, c=0.9977622539852008}, shape=GeoCircle: {center=[X=0.026984641212903917, Y=0.06253120083527987, Z=-0.9954507743974911], radius=1.1693705988362009(67.00000000000001)}}
           [junit4]   1> bboxshape=Rect(minX=-180.0,maxX=180.0,minY=-90.0,maxY=-86.07612418550002)
           [junit4] FAILURE 0.17s | Geo3dWSG84ShapeRectRelationTest.testFailure <<<
           [junit4]    > Throwable #1: java.lang.AssertionError
           [junit4]    > 	at __randomizedtesting.SeedInfo.seed([DEB9AAFD9D348562:B6045911A704E7CD]:0)
           [junit4]    > 	at org.apache.lucene.spatial.spatial4j.Geo3dWSG84ShapeRectRelationTest.testFailure(Geo3dWSG84ShapeRectRelationTest.java:87)
        

        The failure is because Spatial4j thinks that bboxShape.relate(spatialRect) == SpatialRelation.DISJOINT, which seems to be the case:

        spatialRect=Rect(minX=82.0,maxX=92.0,minY=-86.0,maxY=-72.0)
        

        and

        bboxshape=Rect(minX=-180.0,maxX=180.0,minY=-90.0,maxY=-86.07612418550002)
        

        Debugging now...

        Show
        Karl Wright added a comment - - edited Hmm. Trying to introduce a random test for WGS84 using the same infrastructure as for a SPHERE, and getting the following: [junit4] 1> bbox=GeoSouthLatitudeZone: {toplat=-1.5024863073176973(-86.08612418550003)} [junit4] 1> spatialRect=Rect(minX=82.0,maxX=92.0,minY=-86.0,maxY=-72.0), circleShape=Geo3dShape{planetmodel=PlanetModel: {a=1.0011188180710464, b=1.0011188180710464, c=0.9977622539852008}, shape=GeoCircle: {center=[X=0.026984641212903917, Y=0.06253120083527987, Z=-0.9954507743974911], radius=1.1693705988362009(67.00000000000001)}} [junit4] 1> bboxshape=Rect(minX=-180.0,maxX=180.0,minY=-90.0,maxY=-86.07612418550002) [junit4] FAILURE 0.17s | Geo3dWSG84ShapeRectRelationTest.testFailure <<< [junit4] > Throwable #1: java.lang.AssertionError [junit4] > at __randomizedtesting.SeedInfo.seed([DEB9AAFD9D348562:B6045911A704E7CD]:0) [junit4] > at org.apache.lucene.spatial.spatial4j.Geo3dWSG84ShapeRectRelationTest.testFailure(Geo3dWSG84ShapeRectRelationTest.java:87) The failure is because Spatial4j thinks that bboxShape.relate(spatialRect) == SpatialRelation.DISJOINT, which seems to be the case: spatialRect=Rect(minX=82.0,maxX=92.0,minY=-86.0,maxY=-72.0) and bboxshape=Rect(minX=-180.0,maxX=180.0,minY=-90.0,maxY=-86.07612418550002) Debugging now...
        Hide
        Karl Wright added a comment -

        Found the issue, but it will take me some reworking of bounds computations to fix it properly.

        Show
        Karl Wright added a comment - Found the issue, but it will take me some reworking of bounds computations to fix it properly.
        Hide
        Karl Wright added a comment -

        Fixing all remaining bugs and adding a random test for WGS84 model as well.

        Note that I had to give up on having an ellipsoid with different scaling factors for x and y. This is because a "latitude slice" is not actually a slice on such worlds, and therefore GeoBBox's don't actually work.

        There is one more change I'd like to explore having to do with consistent determination of GeoPath width. After that I will try to get someone's attention so this can be committed.

        Show
        Karl Wright added a comment - Fixing all remaining bugs and adding a random test for WGS84 model as well. Note that I had to give up on having an ellipsoid with different scaling factors for x and y. This is because a "latitude slice" is not actually a slice on such worlds, and therefore GeoBBox's don't actually work. There is one more change I'd like to explore having to do with consistent determination of GeoPath width. After that I will try to get someone's attention so this can be committed.
        Hide
        Karl Wright added a comment -

        Ok, all done.

        Show
        Karl Wright added a comment - Ok, all done.
        Hide
        Karl Wright added a comment - - edited

        Note that the general ellipsoid treatment will be some percentage slower than the prior sphere treatment. The critical parts (e.g. shape membership) are unchanged, but the following have taken a small hit:

        • initial setup of objects, e.g. computing points from lat/lon
        • computing the bounds of shapes
        • computing the arc distance between two points

        I have not added a "surface distance" abstraction at this time, in part because there is no accurate ellipsoidal calculation I can find, only approximations. So distance measures continue to be computed as arc distances, normal distances, or direct (linear) distances. Should an accurate surface calculation become available this can readily be added.

        (Further research shows that computing elliptical curve distances are in fact part of a class of problem under the name of "elliptic integrals", which has only numeric solutions. So it is unlikely that we will be implementing this in geo3d soon.)

        Show
        Karl Wright added a comment - - edited Note that the general ellipsoid treatment will be some percentage slower than the prior sphere treatment. The critical parts (e.g. shape membership) are unchanged, but the following have taken a small hit: initial setup of objects, e.g. computing points from lat/lon computing the bounds of shapes computing the arc distance between two points I have not added a "surface distance" abstraction at this time, in part because there is no accurate ellipsoidal calculation I can find, only approximations. So distance measures continue to be computed as arc distances, normal distances, or direct (linear) distances. Should an accurate surface calculation become available this can readily be added. (Further research shows that computing elliptical curve distances are in fact part of a class of problem under the name of "elliptic integrals", which has only numeric solutions. So it is unlikely that we will be implementing this in geo3d soon.)
        Hide
        Karl Wright added a comment -

        Update patch according to comments from dsmiley

        Show
        Karl Wright added a comment - Update patch according to comments from dsmiley
        Hide
        Karl Wright added a comment -

        Patch correcting a random test failure. The bounding box slop between spatial4j and geo3d needs to be larger to handle WGS84.

        Show
        Karl Wright added a comment - Patch correcting a random test failure. The bounding box slop between spatial4j and geo3d needs to be larger to handle WGS84.
        Hide
        Karl Wright added a comment -

        Fixed problem with GeoPaths on ellipsoids; also implemented some suggestions from David Smiley.

        Show
        Karl Wright added a comment - Fixed problem with GeoPaths on ellipsoids; also implemented some suggestions from David Smiley.
        Hide
        David Smiley added a comment -

        Karl, I looked over your latest patch and made some modifications as follows (now attached to this issue):

        • GeoBaseBBox.isShapeInsideBBox: added optimization to exit early if found some inside & outside
        • PlanetModel.hashCode: optimize to not create new Double instances
        • PlanetModel.toString(): prints SPHERE or WGS84 if applicable
        • PlanetModel: added javadoc.
        • Geo3dWGS84ShapeRectRelationTest: removed tabs
        • I refactored the two tests extending RandomizedShapeTestCase to a new Geo3dShapeRectRelationTestCase because they share so much in common.

        "ant precommit" now passes. Tests pass... though I'd like to run some more iterations which I'll do later.

        Other comments:

        GeoPoint.magnitude is a bit confusing to me… it seems it’s lazy initialized. I think it could use comments to this effect? And why is there a computeMagnitude() method distinct from super.magnitude() which also “computes” the magnitude? I’m not saying I think there’s a bug in all this, just that, at a minimum, there should be clarifying comments because it’s confusing.

        Geo3dShapeRectRelationTestCase.geoPointToSpatial4jPoint (something I wrote) is probably wrong since it uses ‘x’ and ‘y’ as if it’s lon & lat when it’s not; and it doesn’t use ‘z’. How should this be rewritten?

        Show
        David Smiley added a comment - Karl, I looked over your latest patch and made some modifications as follows (now attached to this issue): GeoBaseBBox.isShapeInsideBBox: added optimization to exit early if found some inside & outside PlanetModel.hashCode: optimize to not create new Double instances PlanetModel.toString(): prints SPHERE or WGS84 if applicable PlanetModel: added javadoc. Geo3dWGS84ShapeRectRelationTest: removed tabs I refactored the two tests extending RandomizedShapeTestCase to a new Geo3dShapeRectRelationTestCase because they share so much in common. "ant precommit" now passes. Tests pass... though I'd like to run some more iterations which I'll do later. Other comments: GeoPoint.magnitude is a bit confusing to me… it seems it’s lazy initialized. I think it could use comments to this effect? And why is there a computeMagnitude() method distinct from super.magnitude() which also “computes” the magnitude? I’m not saying I think there’s a bug in all this, just that, at a minimum, there should be clarifying comments because it’s confusing. Geo3dShapeRectRelationTestCase.geoPointToSpatial4jPoint (something I wrote) is probably wrong since it uses ‘x’ and ‘y’ as if it’s lon & lat when it’s not; and it doesn’t use ‘z’. How should this be rewritten?
        Hide
        Karl Wright added a comment -

        GeoPoint.magnitude is a bit confusing to me… it seems it’s lazy initialized. I think it could use comments to this effect? And why is there a computeMagnitude() method distinct from super.magnitude() which also “computes” the magnitude?

        Two different kinds of magnitude. One is the inverse ellipsoid magnitude (computeMagnitude()) and the other is the classic root-of-squares linear magnitude. I can change the method name to clarify this and add comments accordingly.

        Geo3dShapeRectRelationTestCase.geoPointToSpatial4jPoint (something I wrote) is probably wrong since it uses ‘x’ and ‘y’ as if it’s lon & lat when it’s not; and it doesn’t use ‘z’. How should this be rewritten?

        I can propose code if you like, but basically what you do is this: (1) compute the inverse magnitude (im), then (2) lat = im * asin(z), (3) lon = atan2(y,x).

        I can code up all of this stuff if you want to propose a good way to deliver it.

        Show
        Karl Wright added a comment - GeoPoint.magnitude is a bit confusing to me… it seems it’s lazy initialized. I think it could use comments to this effect? And why is there a computeMagnitude() method distinct from super.magnitude() which also “computes” the magnitude? Two different kinds of magnitude. One is the inverse ellipsoid magnitude (computeMagnitude()) and the other is the classic root-of-squares linear magnitude. I can change the method name to clarify this and add comments accordingly. Geo3dShapeRectRelationTestCase.geoPointToSpatial4jPoint (something I wrote) is probably wrong since it uses ‘x’ and ‘y’ as if it’s lon & lat when it’s not; and it doesn’t use ‘z’. How should this be rewritten? I can propose code if you like, but basically what you do is this: (1) compute the inverse magnitude (im), then (2) lat = im * asin(z), (3) lon = atan2(y,x). I can code up all of this stuff if you want to propose a good way to deliver it.
        Hide
        David Smiley added a comment -

        I can propose code if you like, but basically what you do is this: (1) compute the inverse magnitude (im), then (2) lat = im * asin(z), (3) lon = atan2(y,x).

        It'd be great if a GeoPoint could compute this – after all it takes lat & lon parameters.

        I can code up all of this stuff if you want to propose a good way to deliver it.

        Do you mean, how literally should you provide the code? Whatever you'd like (patch, ReviewBoard, GitHub PR).

        p.s. I ran a lot more iterations and no errors

        Show
        David Smiley added a comment - I can propose code if you like, but basically what you do is this: (1) compute the inverse magnitude (im), then (2) lat = im * asin(z), (3) lon = atan2(y,x). It'd be great if a GeoPoint could compute this – after all it takes lat & lon parameters. I can code up all of this stuff if you want to propose a good way to deliver it. Do you mean, how literally should you provide the code? Whatever you'd like (patch, ReviewBoard, GitHub PR). p.s. I ran a lot more iterations and no errors
        Hide
        Karl Wright added a comment -

        Applying your patch against trunk yields:

            [mkdir] Created dir: C:\wip\lucene\lucene-6487\lucene\build\spatial\classes\java
            [javac] Compiling 106 source files to C:\wip\lucene\lucene-6487\lucene\build\spatial\classes\java
            [javac] C:\wip\lucene\lucene-6487\lucene\spatial\src\java\org\apache\lucene\spatial\spatial4j\geo3d\GeoDegenerateHorizontalLine.java:27: error: cannot find symbol
            [javac] public class GeoDegenerateHorizontalLine extends GeoBaseBBox {
            [javac]                                                  ^
            [javac]   symbol: class GeoBaseBBox
            [javac] C:\wip\lucene\lucene-6487\lucene\spatial\src\java\org\apache\lucene\spatial\spatial4j\geo3d\GeoDegenerateLatitudeZone.java:26: error: cannot find symbol
            [javac] public class GeoDegenerateLatitudeZone extends GeoBaseBBox {
            [javac]                                                ^
            [javac]   symbol: class GeoBaseBBox
            [javac] C:\wip\lucene\lucene-6487\lucene\spatial\src\java\org\apache\lucene\spatial\spatial4j\geo3d\GeoDegenerateLongitudeSlice.java:25: error: cannot find symbol
            [javac] public class GeoDegenerateLongitudeSlice extends GeoBaseBBox {
            [javac]                                                  ^
            [javac]   symbol: class GeoBaseBBox
            [javac] C:\wip\lucene\lucene-6487\lucene\spatial\src\java\org\apache\lucene\spatial\spatial4j\geo3d\GeoDegenerateVerticalLine.java:25: error: cannot find symbol
            [javac] public class GeoDegenerateVerticalLine extends GeoBaseBBox {
            [javac]                                                ^
            [javac]   symbol: class GeoBaseBBox
            [javac] C:\wip\lucene\lucene-6487\lucene\spatial\src\java\org\apache\lucene\spatial\spatial4j\geo3d\GeoLatitudeZone.java:25: error: cannot find symbol
            [javac] public class GeoLatitudeZone extends GeoBaseBBox {
            [javac]                                      ^
            [javac]   symbol: class GeoBaseBBox
            [javac] C:\wip\lucene\lucene-6487\lucene\spatial\src\java\org\apache\lucene\spatial\spatial4j\geo3d\GeoLongitudeSlice.java:27: error: cannot find symbol
            [javac] public class GeoLongitudeSlice extends GeoBaseBBox {
            [javac]                                        ^
            [javac]   symbol: class GeoBaseBBox
            [javac] C:\wip\lucene\lucene-6487\lucene\spatial\src\java\org\apache\lucene\spatial\spatial4j\geo3d\GeoNorthLatitudeZone.java:25: error: cannot find symbol
            [javac] public class GeoNorthLatitudeZone extends GeoBaseBBox {
            [javac]                                           ^
            [javac]   symbol: class GeoBaseBBox
            [javac] C:\wip\lucene\lucene-6487\lucene\spatial\src\java\org\apache\lucene\spatial\spatial4j\geo3d\GeoNorthRectangle.java:28: error: cannot find symbol
            [javac] public class GeoNorthRectangle extends GeoBaseBBox {
        ...
        

        Clearly we are out of sync again.

        I'm attaching a new patch with my changes, but they do not include your changes for that reason.

        Show
        Karl Wright added a comment - Applying your patch against trunk yields: [mkdir] Created dir: C:\wip\lucene\lucene-6487\lucene\build\spatial\classes\java [javac] Compiling 106 source files to C:\wip\lucene\lucene-6487\lucene\build\spatial\classes\java [javac] C:\wip\lucene\lucene-6487\lucene\spatial\src\java\org\apache\lucene\spatial\spatial4j\geo3d\GeoDegenerateHorizontalLine.java:27: error: cannot find symbol [javac] public class GeoDegenerateHorizontalLine extends GeoBaseBBox { [javac] ^ [javac] symbol: class GeoBaseBBox [javac] C:\wip\lucene\lucene-6487\lucene\spatial\src\java\org\apache\lucene\spatial\spatial4j\geo3d\GeoDegenerateLatitudeZone.java:26: error: cannot find symbol [javac] public class GeoDegenerateLatitudeZone extends GeoBaseBBox { [javac] ^ [javac] symbol: class GeoBaseBBox [javac] C:\wip\lucene\lucene-6487\lucene\spatial\src\java\org\apache\lucene\spatial\spatial4j\geo3d\GeoDegenerateLongitudeSlice.java:25: error: cannot find symbol [javac] public class GeoDegenerateLongitudeSlice extends GeoBaseBBox { [javac] ^ [javac] symbol: class GeoBaseBBox [javac] C:\wip\lucene\lucene-6487\lucene\spatial\src\java\org\apache\lucene\spatial\spatial4j\geo3d\GeoDegenerateVerticalLine.java:25: error: cannot find symbol [javac] public class GeoDegenerateVerticalLine extends GeoBaseBBox { [javac] ^ [javac] symbol: class GeoBaseBBox [javac] C:\wip\lucene\lucene-6487\lucene\spatial\src\java\org\apache\lucene\spatial\spatial4j\geo3d\GeoLatitudeZone.java:25: error: cannot find symbol [javac] public class GeoLatitudeZone extends GeoBaseBBox { [javac] ^ [javac] symbol: class GeoBaseBBox [javac] C:\wip\lucene\lucene-6487\lucene\spatial\src\java\org\apache\lucene\spatial\spatial4j\geo3d\GeoLongitudeSlice.java:27: error: cannot find symbol [javac] public class GeoLongitudeSlice extends GeoBaseBBox { [javac] ^ [javac] symbol: class GeoBaseBBox [javac] C:\wip\lucene\lucene-6487\lucene\spatial\src\java\org\apache\lucene\spatial\spatial4j\geo3d\GeoNorthLatitudeZone.java:25: error: cannot find symbol [javac] public class GeoNorthLatitudeZone extends GeoBaseBBox { [javac] ^ [javac] symbol: class GeoBaseBBox [javac] C:\wip\lucene\lucene-6487\lucene\spatial\src\java\org\apache\lucene\spatial\spatial4j\geo3d\GeoNorthRectangle.java:28: error: cannot find symbol [javac] public class GeoNorthRectangle extends GeoBaseBBox { ... Clearly we are out of sync again. I'm attaching a new patch with my changes, but they do not include your changes for that reason.
        Hide
        Karl Wright added a comment - - edited

        Suggestion:
        (1) Create a branch. It's the only way to really do extensive changes with svn.
        (2) Commit my changes.
        (3) Make your changes and commit those.
        (4) We can iterate as needed.

        Also, FWIW, the only three files I touched were:
        Plane.java
        Vector.java
        GeoPoint.java

        Show
        Karl Wright added a comment - - edited Suggestion: (1) Create a branch. It's the only way to really do extensive changes with svn. (2) Commit my changes. (3) Make your changes and commit those. (4) We can iterate as needed. Also, FWIW, the only three files I touched were: Plane.java Vector.java GeoPoint.java
        Hide
        Karl Wright added a comment -

        Also, when I copy the GeoBaseBBox.java class into place, and delete the old GeoBBoxBase class, I still get:

            [javac] C:\wip\lucene\lucene-6487\lucene\spatial\src\test\org\apache\lucene\spatial\spatial4j\Geo3dShapeRectRelationTest.java:34: error: class Geo3dShapeSphereModelRectRelationTest is public, should be declared in a file named Geo3dShapeSphereModelRectRelationTest.java
            [javac] public class Geo3dShapeSphereModelRectRelationTest extends Geo3dShapeRectRelationTestCase {
            [javac]        ^
            [javac] 1 error
        

        I'm assuming you intended to copy the former class into the latter but somehow overwrote the former? I'll try that and see if I can get it to build...

        Show
        Karl Wright added a comment - Also, when I copy the GeoBaseBBox.java class into place, and delete the old GeoBBoxBase class, I still get: [javac] C:\wip\lucene\lucene-6487\lucene\spatial\src\test\org\apache\lucene\spatial\spatial4j\Geo3dShapeRectRelationTest.java:34: error: class Geo3dShapeSphereModelRectRelationTest is public , should be declared in a file named Geo3dShapeSphereModelRectRelationTest.java [javac] public class Geo3dShapeSphereModelRectRelationTest extends Geo3dShapeRectRelationTestCase { [javac] ^ [javac] 1 error I'm assuming you intended to copy the former class into the latter but somehow overwrote the former? I'll try that and see if I can get it to build...
        Hide
        Karl Wright added a comment -

        Resolved outstanding build issues from David's patch and added what was requested.

        Show
        Karl Wright added a comment - Resolved outstanding build issues from David's patch and added what was requested.
        Hide
        ASF subversion and git services added a comment -

        Commit 1681907 from David Smiley in branch 'dev/branches/lucene6487'
        [ https://svn.apache.org/r1681907 ]

        LUCENE-6487: Geo3D with WGS84 in-progress with David's mods
        (PlanetModel, and refactor of Geo3dShapeRectRelationTestCase)

        Show
        ASF subversion and git services added a comment - Commit 1681907 from David Smiley in branch 'dev/branches/lucene6487' [ https://svn.apache.org/r1681907 ] LUCENE-6487 : Geo3D with WGS84 in-progress with David's mods (PlanetModel, and refactor of Geo3dShapeRectRelationTestCase)
        Hide
        David Smiley added a comment -

        Suggestion: (1) Create a branch., ...

        Great suggestion – I did those steps.

        It's a shame the patch didn't deal well with the rename of GeoBaseBBox.

        Show
        David Smiley added a comment - Suggestion: (1) Create a branch., ... Great suggestion – I did those steps. It's a shame the patch didn't deal well with the rename of GeoBaseBBox.
        Hide
        ASF subversion and git services added a comment -

        Commit 1682021 from David Smiley in branch 'dev/branches/lucene6487'
        [ https://svn.apache.org/r1682021 ]

        LUCENE-6487: Geo3D with WGS84 patch from Karl: GeoPoint.getLat & getLon.

        Show
        ASF subversion and git services added a comment - Commit 1682021 from David Smiley in branch 'dev/branches/lucene6487' [ https://svn.apache.org/r1682021 ] LUCENE-6487 : Geo3D with WGS84 patch from Karl: GeoPoint.getLat & getLon.
        Hide
        David Smiley added a comment -

        This looks ready to me. I do have a couple lingering thoughts (not blockers):

        • Might it make sense for Vector to to have it's x, y, and z, plus GeoPoint's magnitude as floats instead of doubles? I was just thinking the objects are comparatively heavy compared to a lat-lon based Point.
        • It'd be nice if there was some random round-trip tests from lat & lon to GeoPoint to back again, checking the result is within a tolerance.

        Nicholas Knize do you have an interest in reviewing the Geo3d lucene6487 branch with respect to WGS84 support?

        Show
        David Smiley added a comment - This looks ready to me. I do have a couple lingering thoughts ( not blockers ): Might it make sense for Vector to to have it's x, y, and z, plus GeoPoint's magnitude as floats instead of doubles? I was just thinking the objects are comparatively heavy compared to a lat-lon based Point. It'd be nice if there was some random round-trip tests from lat & lon to GeoPoint to back again, checking the result is within a tolerance. Nicholas Knize do you have an interest in reviewing the Geo3d lucene6487 branch with respect to WGS84 support?
        Hide
        Karl Wright added a comment -

        Simple round-trip tests are trivial, but I won't get to one today, and I agree that it is not a blocker for commit. We can create a separate ticket if you go ahead and merge.

        re doubles vs. floats: Well, the actual (x,y,z) value needs to be accurate to within 1e-12 in order to properly specify various planes etc throughout the module. So I'd be cautious about deliberately reducing precision, since geopoint extends vector and plane does too. We can experiment but... once again, I think a separate ticket would be in order.

        Show
        Karl Wright added a comment - Simple round-trip tests are trivial, but I won't get to one today, and I agree that it is not a blocker for commit. We can create a separate ticket if you go ahead and merge. re doubles vs. floats: Well, the actual (x,y,z) value needs to be accurate to within 1e-12 in order to properly specify various planes etc throughout the module. So I'd be cautious about deliberately reducing precision, since geopoint extends vector and plane does too. We can experiment but... once again, I think a separate ticket would be in order.
        Hide
        Karl Wright added a comment -

        I added a test (and actually found a typo when I did that!) so that was a good call.

        Show
        Karl Wright added a comment - I added a test (and actually found a typo when I did that!) so that was a good call.
        Hide
        ASF subversion and git services added a comment -

        Commit 1682357 from David Smiley in branch 'dev/branches/lucene6487'
        [ https://svn.apache.org/r1682357 ]

        LUCENE-6487: Geo3D with WGS84 patch from Karl: fix bug in GeoPoint.getLongitude with test
        from https://reviews.apache.org/r/34744/diff/raw/

        Show
        ASF subversion and git services added a comment - Commit 1682357 from David Smiley in branch 'dev/branches/lucene6487' [ https://svn.apache.org/r1682357 ] LUCENE-6487 : Geo3D with WGS84 patch from Karl: fix bug in GeoPoint.getLongitude with test from https://reviews.apache.org/r/34744/diff/raw/
        Hide
        ASF subversion and git services added a comment -

        Commit 1682359 from David Smiley in branch 'dev/branches/lucene6487'
        [ https://svn.apache.org/r1682359 ]

        LUCENE-6487: Geo3D with WGS84: randomize GeoPointTest lat-lon round-trip

        Show
        ASF subversion and git services added a comment - Commit 1682359 from David Smiley in branch 'dev/branches/lucene6487' [ https://svn.apache.org/r1682359 ] LUCENE-6487 : Geo3D with WGS84: randomize GeoPointTest lat-lon round-trip
        Hide
        Karl Wright added a comment -

        Hi David,

        This test is now wrong, and will blow up whenever a point is chosen at the poles:

            final double pLat = (randomFloat() * 180.0 - 90.0) * DistanceUtils.DEGREES_TO_RADIANS;
            final double pLon = (randomFloat() * 360.0 - 180.0) * DistanceUtils.DEGREES_TO_RADIANS;
            final GeoPoint p1 = new GeoPoint(PlanetModel.SPHERE, pLat, pLon);
            assertEquals(pLat, p1.getLatitude(), 1e-12);
            assertEquals(pLon, p1.getLongitude(), 1e-12);
            final GeoPoint p2 = new GeoPoint(PlanetModel.WGS84, pLat, pLon);
            assertEquals(pLat, p2.getLatitude(), 1e-12);
            assertEquals(pLon, p2.getLongitude(), 1e-12);
        

        The conversion at the pole will produce a longitude value always of zero, not what went into it.

        Show
        Karl Wright added a comment - Hi David, This test is now wrong, and will blow up whenever a point is chosen at the poles: final double pLat = (randomFloat() * 180.0 - 90.0) * DistanceUtils.DEGREES_TO_RADIANS; final double pLon = (randomFloat() * 360.0 - 180.0) * DistanceUtils.DEGREES_TO_RADIANS; final GeoPoint p1 = new GeoPoint(PlanetModel.SPHERE, pLat, pLon); assertEquals(pLat, p1.getLatitude(), 1e-12); assertEquals(pLon, p1.getLongitude(), 1e-12); final GeoPoint p2 = new GeoPoint(PlanetModel.WGS84, pLat, pLon); assertEquals(pLat, p2.getLatitude(), 1e-12); assertEquals(pLon, p2.getLongitude(), 1e-12); The conversion at the pole will produce a longitude value always of zero, not what went into it.
        Hide
        David Smiley added a comment -

        Thanks! Instead what I should measure is that the distance between the original point and the round-trip point is close to 0.

        Show
        David Smiley added a comment - Thanks! Instead what I should measure is that the distance between the original point and the round-trip point is close to 0.
        Hide
        David Smiley added a comment -

        I rewrote the test; and indirectly test the arcDistance somewhat since it calls that. Can you try this? It seems my error epsilons are too tiny. Or maybe you see something the matter.

        
          @Test
          public void testConversion() {
            testPointRoundTrip(PlanetModel.SPHERE, 90, 0, 1e-12);
            testPointRoundTrip(PlanetModel.SPHERE, -90, 0, 1e-12);
            testPointRoundTrip(PlanetModel.WGS84, 90, 0, 1e-12);
            testPointRoundTrip(PlanetModel.WGS84, -90, 0, 1e-12);
        
            final double pLat = (randomFloat() * 180.0 - 90.0) * DistanceUtils.DEGREES_TO_RADIANS;
            final double pLon = (randomFloat() * 360.0 - 180.0) * DistanceUtils.DEGREES_TO_RADIANS;
            testPointRoundTrip(PlanetModel.SPHERE, pLat, pLon, 1e-12);
            testPointRoundTrip(PlanetModel.WGS84, pLat, pLon, 1e-6);//bigger error tolerance
          }
        
          protected void testPointRoundTrip(PlanetModel planetModel, double pLat, double pLon, double epsilon) {
            final GeoPoint p1 = new GeoPoint(planetModel, pLat, pLon);
            final GeoPoint p2 = new GeoPoint(planetModel, p1.getLatitude(), p1.getLongitude());
            double dist = p1.arcDistance(p2);
            assertEquals(0, dist, epsilon);
          }
        
        
        Show
        David Smiley added a comment - I rewrote the test; and indirectly test the arcDistance somewhat since it calls that. Can you try this? It seems my error epsilons are too tiny. Or maybe you see something the matter. @Test public void testConversion() { testPointRoundTrip(PlanetModel.SPHERE, 90, 0, 1e-12); testPointRoundTrip(PlanetModel.SPHERE, -90, 0, 1e-12); testPointRoundTrip(PlanetModel.WGS84, 90, 0, 1e-12); testPointRoundTrip(PlanetModel.WGS84, -90, 0, 1e-12); final double pLat = (randomFloat() * 180.0 - 90.0) * DistanceUtils.DEGREES_TO_RADIANS; final double pLon = (randomFloat() * 360.0 - 180.0) * DistanceUtils.DEGREES_TO_RADIANS; testPointRoundTrip(PlanetModel.SPHERE, pLat, pLon, 1e-12); testPointRoundTrip(PlanetModel.WGS84, pLat, pLon, 1e-6); //bigger error tolerance } protected void testPointRoundTrip(PlanetModel planetModel, double pLat, double pLon, double epsilon) { final GeoPoint p1 = new GeoPoint(planetModel, pLat, pLon); final GeoPoint p2 = new GeoPoint(planetModel, p1.getLatitude(), p1.getLongitude()); double dist = p1.arcDistance(p2); assertEquals(0, dist, epsilon); }
        Hide
        Karl Wright added a comment -

        I'll have a look. Please bear in mind that trig functions and their inverses, if done in sequence enough, may well wander pretty far off of the original values, so there may be nothing wrong with your test. But I will make sure of that.

        Show
        Karl Wright added a comment - I'll have a look. Please bear in mind that trig functions and their inverses, if done in sequence enough, may well wander pretty far off of the original values, so there may be nothing wrong with your test. But I will make sure of that.
        Hide
        Karl Wright added a comment -

        Ok, what I find is that because the arc distance is in effect taking a
        square root, instead of 1e-12 the error should always be 1e-6. That is
        just the way the math works.

        Karl

        Sent from my Windows Phone
        From: Karl Wright (JIRA)
        Sent: 5/30/2015 6:22 AM
        To: dev@lucene.apache.org
        Subject: [jira] [Commented] (LUCENE-6487) Add WGS84 capability to geo3d
        support

        [ https://issues.apache.org/jira/browse/LUCENE-6487?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14565926#comment-14565926
        ]

        Karl Wright commented on LUCENE-6487:
        -------------------------------------

        I'll have a look. Please bear in mind that trig functions and their
        inverses, if done in sequence enough, may well wander pretty far off
        of the original values, so there may be nothing wrong with your test.
        But I will make sure of that.


        This message was sent by Atlassian JIRA
        (v6.3.4#6332)

        ---------------------------------------------------------------------
        To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
        For additional commands, e-mail: dev-help@lucene.apache.org

        Show
        Karl Wright added a comment - Ok, what I find is that because the arc distance is in effect taking a square root, instead of 1e-12 the error should always be 1e-6. That is just the way the math works. Karl Sent from my Windows Phone From: Karl Wright (JIRA) Sent: 5/30/2015 6:22 AM To: dev@lucene.apache.org Subject: [jira] [Commented] ( LUCENE-6487 ) Add WGS84 capability to geo3d support [ https://issues.apache.org/jira/browse/LUCENE-6487?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14565926#comment-14565926 ] Karl Wright commented on LUCENE-6487 : ------------------------------------- I'll have a look. Please bear in mind that trig functions and their inverses, if done in sequence enough, may well wander pretty far off of the original values, so there may be nothing wrong with your test. But I will make sure of that. – This message was sent by Atlassian JIRA (v6.3.4#6332) --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org For additional commands, e-mail: dev-help@lucene.apache.org
        Hide
        ASF subversion and git services added a comment -

        Commit 1682667 from David Smiley in branch 'dev/branches/lucene6487'
        [ https://svn.apache.org/r1682667 ]

        LUCENE-6487: Geo3D with WGS84: fix GeoPointTest to test via distance

        Show
        ASF subversion and git services added a comment - Commit 1682667 from David Smiley in branch 'dev/branches/lucene6487' [ https://svn.apache.org/r1682667 ] LUCENE-6487 : Geo3D with WGS84: fix GeoPointTest to test via distance
        Hide
        Karl Wright added a comment -

        Hi David,

        As far as I am concerned, this is ready to be committed to trunk.

        If you are waiting for Nicholas, I fully agree, but he's on paternity leave so it may be quite some time. Might be better to commit and revise later, if needed.

        Show
        Karl Wright added a comment - Hi David, As far as I am concerned, this is ready to be committed to trunk. If you are waiting for Nicholas, I fully agree, but he's on paternity leave so it may be quite some time. Might be better to commit and revise later, if needed.
        Hide
        ASF subversion and git services added a comment -

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

        LUCENE-6487: Geo3D with WGS84 option (Merged from lucene6487 + CHANGES.txt)

        Show
        ASF subversion and git services added a comment - Commit 1683100 from David Smiley in branch 'dev/trunk' [ https://svn.apache.org/r1683100 ] LUCENE-6487 : Geo3D with WGS84 option (Merged from lucene6487 + CHANGES.txt)
        Hide
        ASF subversion and git services added a comment -

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

        LUCENE-6487: Geo3D with WGS84 option

        Show
        ASF subversion and git services added a comment - Commit 1683102 from David Smiley in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1683102 ] LUCENE-6487 : Geo3D with WGS84 option
        Hide
        David Smiley added a comment -

        If you are waiting for Nicholas, I fully agree, but he's on paternity leave so it may be quite some time. Might be better to commit and revise later, if needed.

        Committed now.

        It was a nice-to-have, plus I was/am busy.

        Thanks again for WGS-84 support! You made it look so easy

        Show
        David Smiley added a comment - If you are waiting for Nicholas, I fully agree, but he's on paternity leave so it may be quite some time. Might be better to commit and revise later, if needed. Committed now. It was a nice-to-have, plus I was/am busy. Thanks again for WGS-84 support! You made it look so easy
        Hide
        ASF subversion and git services added a comment -

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

        LUCENE-6487: Fix Java 8 reference to Double.hashCode (use IntelliJ generated hashCode instead)

        Show
        ASF subversion and git services added a comment - Commit 1683115 from David Smiley in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1683115 ] LUCENE-6487 : Fix Java 8 reference to Double.hashCode (use IntelliJ generated hashCode instead)
        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