[GitHub] lucene-solr pull request #246: LUCENE-7951: New wrapper classes for Geo3d

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #246: LUCENE-7951: New wrapper classes for Geo3d

sarxos
GitHub user iverase opened a pull request:

    https://github.com/apache/lucene-solr/pull/246

    LUCENE-7951: New wrapper classes for Geo3d

    Create a pull request for LUCENE-7951

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/iverase/lucene-solr origin/LUCENE-7951

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/lucene-solr/pull/246.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #246
   
----
commit 3ba3dad6b7169b73f3340f518f151511f149e266
Author: ivera <[hidden email]>
Date:   2017-09-07T08:05:18Z

    LUCENE-7951: first version

commit ea31e5ce0f377fe23f2d4446fccb5a8e8c52690d
Author: ivera <[hidden email]>
Date:   2017-09-07T08:09:07Z

    LUCENE-7951: first version

----


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #246: LUCENE-7951: New wrapper classes for Geo3d

sarxos
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/246#discussion_r137824698
 
    --- Diff: lucene/spatial3d/src/test/org/apache/lucene/spatial3d/geom/RandomGeo3dShapeGenerator.java ---
    @@ -149,6 +150,26 @@ public PlanetModel randomPlanetModel() {
       }
     
       /**
    +   * Method that returns a random generated Planet model as string from the supported
    --- End diff --
   
    I don't think you need this method.  I think you are using it only for init(map) when the test using it could instead simply do randomBoolean() to directly set the planet model on the factory.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #246: LUCENE-7951: New wrapper classes for Geo3d

sarxos
In reply to this post by sarxos
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/246#discussion_r137825324
 
    --- Diff: lucene/spatial-extras/src/test/org/apache/lucene/spatial/spatial4j/Geo3dAreaRptTest.java ---
    @@ -40,10 +41,10 @@
     
       SpatialContext ctx;
       Geo3dSpatialContextFactory factory;
    -  RandomGeoShapeGenerator shapeGenerator = new RandomGeoShapeGenerator();
    +  RandomGeo3dShapeGenerator shapeGenerator = new RandomGeo3dShapeGenerator();
     
       private void setupGeohashGrid() {
    -    String planetModel = shapeGenerator.randomPlanetModel();
    +    String planetModel = shapeGenerator.randomStringPlanetModel();
         factory = new Geo3dSpatialContextFactory();
         Map<String, String>  args = new HashMap<>();
         args.put("planetModel", planetModel);
    --- End diff --
   
    I suggest not calling init(); just set the factory.planetModel directly


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr issue #246: LUCENE-7951: New wrapper classes for Geo3d

sarxos
In reply to this post by sarxos
Github user iverase commented on the issue:

    https://github.com/apache/lucene-solr/pull/246
 
    I have added the possibility to relate no geo3d rectangles. I was trying to avoid that but I see that for the time being it is necessary.
   
    I did the changes on the test as well so I am setting the planet model directly to the factory.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #246: LUCENE-7951: New wrapper classes for Geo3d

sarxos
In reply to this post by sarxos
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/246#discussion_r138329683
 
    --- Diff: lucene/spatial-extras/src/java/org/apache/lucene/spatial/spatial4j/Geo3dShape.java ---
    @@ -14,70 +14,69 @@
      * See the License for the specific language governing permissions and
      * limitations under the License.
      */
    +
     package org.apache.lucene.spatial.spatial4j;
     
    +import org.apache.lucene.spatial3d.geom.GeoArea;
    +import org.apache.lucene.spatial3d.geom.GeoAreaFactory;
    +import org.apache.lucene.spatial3d.geom.GeoAreaShape;
    +import org.apache.lucene.spatial3d.geom.GeoBBox;
    +import org.apache.lucene.spatial3d.geom.GeoBBoxFactory;
    +import org.apache.lucene.spatial3d.geom.GeoPoint;
    +import org.apache.lucene.spatial3d.geom.LatLonBounds;
     import org.locationtech.spatial4j.context.SpatialContext;
     import org.locationtech.spatial4j.distance.DistanceUtils;
     import org.locationtech.spatial4j.shape.Point;
     import org.locationtech.spatial4j.shape.Rectangle;
     import org.locationtech.spatial4j.shape.Shape;
     import org.locationtech.spatial4j.shape.SpatialRelation;
    -import org.locationtech.spatial4j.shape.impl.RectangleImpl;
    -import org.apache.lucene.spatial3d.geom.LatLonBounds;
    -import org.apache.lucene.spatial3d.geom.GeoArea;
    -import org.apache.lucene.spatial3d.geom.GeoAreaFactory;
    -import org.apache.lucene.spatial3d.geom.GeoPoint;
    -import org.apache.lucene.spatial3d.geom.GeoShape;
    -import org.apache.lucene.spatial3d.geom.PlanetModel;
     
     /**
    - * A Spatial4j Shape wrapping a {@link GeoShape} ("Geo3D") -- a 3D planar geometry based Spatial4j Shape implementation.
    + * A Spatial4j Shape wrapping a {@link GeoAreaShape} ("Geo3D") -- a 3D planar geometry
    + * based Spatial4j Shape implementation.
      * Geo3D implements shapes on the surface of a sphere or ellipsoid.
      *
    + * @param <T> is the type of {@link GeoAreaShape}
    + *
      * @lucene.experimental
      */
    -public class Geo3dShape implements Shape {
    -  /** The required size of this adjustment depends on the actual planetary model chosen.
    -   * This value is big enough to account for WGS84. */
    -  protected static final double ROUNDOFF_ADJUSTMENT = 0.05;
     
    -  public final SpatialContext ctx;
    -  public final GeoShape shape;
    -  public final PlanetModel planetModel;
    +public class Geo3dShape<T extends GeoAreaShape> implements Shape {
     
    -  private volatile Rectangle boundingBox = null; // lazy initialized
    +  protected final SpatialContext spatialcontext;
     
    -  public Geo3dShape(final GeoShape shape, final SpatialContext ctx) {
    -    this(PlanetModel.SPHERE, shape, ctx);
    -  }
    +  protected T shape;
    +  protected volatile Rectangle boundingBox = null; // lazy initialized
    +  protected volatile Point center = null; // lazy initialized
     
    -  public Geo3dShape(final PlanetModel planetModel, final GeoShape shape, final SpatialContext ctx) {
    -    if (!ctx.isGeo()) {
    -      throw new IllegalArgumentException("SpatialContext.isGeo() must be true");
    -    }
    -    this.ctx = ctx;
    -    this.planetModel = planetModel;
    +  public Geo3dShape(final T shape, final SpatialContext spatialcontext){
    +    this.spatialcontext = spatialcontext;
         this.shape = shape;
       }
     
       @Override
    -  public SpatialContext getContext() {
    -    return ctx;
    -  }
    -
    -  @Override
       public SpatialRelation relate(Shape other) {
    -    if (other instanceof Rectangle)
    -      return relate((Rectangle)other);
    -    else if (other instanceof Point)
    -      return relate((Point)other);
    -    else
    -      throw new RuntimeException("Unimplemented shape relationship determination: " + other.getClass());
    +    if (other instanceof Geo3dShape<?>){
    +      int relationship = shape.getRelationship(((Geo3dShape<?>)other).shape);
    --- End diff --
   
    in the keeping of the style of other Shape classes, I suggest extracting a public overloaded method accepting Geo3dShape type


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #246: LUCENE-7951: New wrapper classes for Geo3d

sarxos
In reply to this post by sarxos
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/246#discussion_r138328277
 
    --- Diff: lucene/spatial-extras/src/java/org/apache/lucene/spatial/spatial4j/Geo3dDistanceCalculator.java ---
    @@ -70,7 +71,22 @@ public boolean within(Point from, double toX, double toY, double distance) {
     
       @Override
       public Point pointOnBearing(Point from, double distDEG, double bearingDEG, SpatialContext ctx, Point reuse) {
    -    throw new UnsupportedOperationException();
    +    double dist = DistanceUtils.DEGREES_TO_RADIANS * distDEG;
    +    double bearing = DistanceUtils.DEGREES_TO_RADIANS * bearingDEG;
    +    Geo3dPointShape geoFrom = (Geo3dPointShape) from;
    +    GeoPoint point = (GeoPoint)geoFrom.shape;
    +
    --- End diff --
   
    Can you please add the name of this formula (if there is one) and/or a URL for further info?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #246: LUCENE-7951: New wrapper classes for Geo3d

sarxos
In reply to this post by sarxos
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/246#discussion_r138329027
 
    --- Diff: lucene/spatial-extras/src/java/org/apache/lucene/spatial/spatial4j/Geo3dRectangleShape.java ---
    @@ -30,32 +30,49 @@
     import org.locationtech.spatial4j.shape.SpatialRelation;
     
     /**
    - * Specialization of a {@link Geo3dAreaShape} which represents a {@link Rectangle}. This
    + * Specialization of a {@link Geo3dShape} which represents a {@link Rectangle}. This
      * class is used for geohashing.
      *
      * @lucene.experimental
      */
    -public class Geo3dRectangleShape extends Geo3dAreaShape<GeoBBox> implements Rectangle {
    -
    -  //bounds object to return the max and min latitude and longitude
    -  private volatile  LatLonBounds bounds; // lazy initialized
    +public class Geo3dRectangleShape extends Geo3dShape<GeoBBox> implements Rectangle {
    +
    +  private double minX;
    +  private double maxX;
    +  private double minY;
    +  private double maxY;
    +
    +  public Geo3dRectangleShape(final GeoBBox shape,
    +                             final SpatialContext spatialcontext,
    +                             double minX,
    +                             double maxX,
    +                             double minY,
    +                             double maxY) {
    +    super(shape, spatialcontext);
    +    this.minX = minX;
    +    this.maxX = maxX;
    +    this.minY = minY;
    +    this.maxY = maxY;
    +  }
     
       public Geo3dRectangleShape(final GeoBBox shape, final SpatialContext spatialcontext) {
         super(shape, spatialcontext);
    +    setBounds();
       }
     
    +
    +
       /**
    -   * Get the bounds from the wrapped GeoBBox.
    +   * Set the bounds from the wrapped GeoBBox.
        * @return The bounds
        */
    -  private LatLonBounds getBounds() {
    -    LatLonBounds bounds  = this.bounds;//volatile read once
    -    if (bounds == null) {
    -      bounds = new LatLonBounds();
    -      shape.getBounds(bounds);
    -      this.bounds = bounds;
    -    }
    -    return bounds;
    +  private void setBounds() {
    --- End diff --
   
    minor: if this method accepted the shape field as a parameter, it would be more clear from where the boundaries are coming from.  Or give it a longer name perhaps like setBoundsFromShape.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #246: LUCENE-7951: New wrapper classes for Geo3d

sarxos
In reply to this post by sarxos
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/246#discussion_r138328560
 
    --- Diff: lucene/spatial-extras/src/java/org/apache/lucene/spatial/spatial4j/Geo3dRectangleShape.java ---
    @@ -30,32 +30,49 @@
     import org.locationtech.spatial4j.shape.SpatialRelation;
     
     /**
    - * Specialization of a {@link Geo3dAreaShape} which represents a {@link Rectangle}. This
    + * Specialization of a {@link Geo3dShape} which represents a {@link Rectangle}. This
      * class is used for geohashing.
    --- End diff --
   
    minor: not need to say "this class is used for geohashing".  It could be used for a variety of purposes, notably a query for a rectangular map area.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #246: LUCENE-7951: New wrapper classes for Geo3d

sarxos
In reply to this post by sarxos
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/246#discussion_r138331920
 
    --- Diff: lucene/spatial-extras/src/java/org/apache/lucene/spatial/spatial4j/Geo3dShapeFactory.java ---
    @@ -125,7 +125,12 @@ public Point pointXY(double x, double y) {
     
       @Override
       public Point pointXYZ(double x, double y, double z) {
    --- End diff --
   
    x,y,z here is horizontal (longitude), vertical (latitude), and some user-defined value.  It's not geo-centric coordinates as is found commonly in Geo3d.  This method is called by the WKT parser (and perhaps GeoJSON?) if the input format has a 3rd dimension to it.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #246: LUCENE-7951: New wrapper classes for Geo3d

sarxos
In reply to this post by sarxos
Github user dsmiley commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/246#discussion_r138331367
 
    --- Diff: lucene/spatial-extras/src/java/org/apache/lucene/spatial/spatial4j/Geo3dShape.java ---
    @@ -96,45 +95,25 @@ else if (relationship == GeoArea.DISJOINT)
       }
     
       protected SpatialRelation relate(Point p) {
    -    // Create a GeoPoint
    -    GeoPoint point = new GeoPoint(planetModel, p.getY()* DistanceUtils.DEGREES_TO_RADIANS, p.getX()* DistanceUtils.DEGREES_TO_RADIANS);
    +    GeoPoint point = new GeoPoint(shape.getPlanetModel(), p.getY()* DistanceUtils.DEGREES_TO_RADIANS, p.getX()* DistanceUtils.DEGREES_TO_RADIANS);
         if (shape.isWithin(point)) {
    -      // Point within shape
           return SpatialRelation.CONTAINS;
         }
         return SpatialRelation.DISJOINT;
       }
     
    -
    -
       @Override
       public Rectangle getBoundingBox() {
         Rectangle bbox = this.boundingBox;//volatile read once
         if (bbox == null) {
           LatLonBounds bounds = new LatLonBounds();
           shape.getBounds(bounds);
    -      double leftLon;
    -      double rightLon;
    -      if (bounds.checkNoLongitudeBound()) {
    -        leftLon = -180.0;
    -        rightLon = 180.0;
    -      } else {
    -        leftLon = bounds.getLeftLongitude().doubleValue() * DistanceUtils.RADIANS_TO_DEGREES;
    -        rightLon = bounds.getRightLongitude().doubleValue() * DistanceUtils.RADIANS_TO_DEGREES;
    -      }
    -      double minLat;
    -      if (bounds.checkNoBottomLatitudeBound()) {
    -        minLat = -90.0;
    -      } else {
    -        minLat = bounds.getMinLatitude().doubleValue() * DistanceUtils.RADIANS_TO_DEGREES;
    -      }
    -      double maxLat;
    -      if (bounds.checkNoTopLatitudeBound()) {
    -        maxLat = 90.0;
    -      } else {
    -        maxLat = bounds.getMaxLatitude().doubleValue() * DistanceUtils.RADIANS_TO_DEGREES;
    -      }
    -      bbox = new RectangleImpl(leftLon, rightLon, minLat, maxLat, ctx).getBuffered(ROUNDOFF_ADJUSTMENT, ctx);
    --- End diff --
   
    I presume you have found this ROUNDOFF_ADJUSTMENT to be no longer necessary for tests to pass?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #246: LUCENE-7951: New wrapper classes for Geo3d

sarxos
In reply to this post by sarxos
Github user iverase commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/246#discussion_r138555768
 
    --- Diff: lucene/spatial-extras/src/java/org/apache/lucene/spatial/spatial4j/Geo3dShape.java ---
    @@ -96,45 +95,25 @@ else if (relationship == GeoArea.DISJOINT)
       }
     
       protected SpatialRelation relate(Point p) {
    -    // Create a GeoPoint
    -    GeoPoint point = new GeoPoint(planetModel, p.getY()* DistanceUtils.DEGREES_TO_RADIANS, p.getX()* DistanceUtils.DEGREES_TO_RADIANS);
    +    GeoPoint point = new GeoPoint(shape.getPlanetModel(), p.getY()* DistanceUtils.DEGREES_TO_RADIANS, p.getX()* DistanceUtils.DEGREES_TO_RADIANS);
         if (shape.isWithin(point)) {
    -      // Point within shape
           return SpatialRelation.CONTAINS;
         }
         return SpatialRelation.DISJOINT;
       }
     
    -
    -
       @Override
       public Rectangle getBoundingBox() {
         Rectangle bbox = this.boundingBox;//volatile read once
         if (bbox == null) {
           LatLonBounds bounds = new LatLonBounds();
           shape.getBounds(bounds);
    -      double leftLon;
    -      double rightLon;
    -      if (bounds.checkNoLongitudeBound()) {
    -        leftLon = -180.0;
    -        rightLon = 180.0;
    -      } else {
    -        leftLon = bounds.getLeftLongitude().doubleValue() * DistanceUtils.RADIANS_TO_DEGREES;
    -        rightLon = bounds.getRightLongitude().doubleValue() * DistanceUtils.RADIANS_TO_DEGREES;
    -      }
    -      double minLat;
    -      if (bounds.checkNoBottomLatitudeBound()) {
    -        minLat = -90.0;
    -      } else {
    -        minLat = bounds.getMinLatitude().doubleValue() * DistanceUtils.RADIANS_TO_DEGREES;
    -      }
    -      double maxLat;
    -      if (bounds.checkNoTopLatitudeBound()) {
    -        maxLat = 90.0;
    -      } else {
    -        maxLat = bounds.getMaxLatitude().doubleValue() * DistanceUtils.RADIANS_TO_DEGREES;
    -      }
    -      bbox = new RectangleImpl(leftLon, rightLon, minLat, maxLat, ctx).getBuffered(ROUNDOFF_ADJUSTMENT, ctx);
    --- End diff --
   
    Yes, it seems not necessary. Not sure what was needed before but I am glad that we can hopefully get rid of that.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] lucene-solr pull request #246: LUCENE-7951: New wrapper classes for Geo3d

sarxos
In reply to this post by sarxos
Github user iverase commented on a diff in the pull request:

    https://github.com/apache/lucene-solr/pull/246#discussion_r138556038
 
    --- Diff: lucene/spatial-extras/src/java/org/apache/lucene/spatial/spatial4j/Geo3dDistanceCalculator.java ---
    @@ -70,7 +71,22 @@ public boolean within(Point from, double toX, double toY, double distance) {
     
       @Override
       public Point pointOnBearing(Point from, double distDEG, double bearingDEG, SpatialContext ctx, Point reuse) {
    -    throw new UnsupportedOperationException();
    +    double dist = DistanceUtils.DEGREES_TO_RADIANS * distDEG;
    +    double bearing = DistanceUtils.DEGREES_TO_RADIANS * bearingDEG;
    +    Geo3dPointShape geoFrom = (Geo3dPointShape) from;
    +    GeoPoint point = (GeoPoint)geoFrom.shape;
    +
    --- End diff --
   
    I have changes the formula to use vincenty's formulae. Slower as it is iterative but It taken into account the planet model.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]