Details

    • Type: Improvement
    • Status: Open
    • Priority: Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: modules/spatial3d
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Hi,

      Working with geosahpes and trying to resolve spatial relationships between them I came accross a big limitation when trying to solve the relationship between two geopolygons. This object does not expose the internal structure. In particular at some point, it is necessary to check if one polygon intersects the edges of the other polygon which currently is not possible as edges are not exposed.

      To be able to perform such operation it can be several options. The ones I can think of are:

      1) Expose the edges of the polygon ( and probably the notable points for the edges) adding getters in the GeoPolygon interface. Easy to implement and leave users the responsability of coding the spatial relationship.

      2) Extends GeoPolygon interface to extends geoarea and leave the object make the spatial relationship.

      3) Extends GeoShape interface so all shapes can infer the spatial relationship with other GeoShapes.

      I might be bias as my interest is in 2d Shapes in the unit sphere and there might be some cases which what I propose cannot be implemented or are againts the aim of the library.

      What do you think?

      Cheers,

      Ignacio

      1. LUCENE-7906-Generator_update.patch
        2 kB
        Ignacio Vera
      2. LUCENE-7906-test.patch
        17 kB
        Ignacio Vera
      3. LUCENE-7906-test.patch
        15 kB
        Ignacio Vera
      4. LUCENE-7906-test.patch
        15 kB
        Ignacio Vera
      5. test-degenerate_point.patch
        1 kB
        Ignacio Vera
      6. LUCENE-7906-AreaShape.patch
        173 kB
        Ignacio Vera
      7. LUCENE-7906.patch
        122 kB
        Ignacio Vera

        Activity

        Hide
        kwright@metacarta.com Karl Wright added a comment -

        Hi Ignacio Vera,

        In Geo3D, there is currently only support for computing the intersection with shapes that implement GeoArea. These only include GeoBBox (2D) and XYZSolid (3D). Currently, GeoPolygon objects do not implement GeoArea. This is by design. There are many cases and corner cases that would need algorithmic development to properly implement polygon intersection.

        If you need to support this case, feel free to supply a patch where GeoPolygon extends GeoArea. You will need to implement getRelationship() for GeoComplexPolygon, GeoConvexPolygon, GeoConcavePolygon, and GeoCompositePolygon. I would also urge you to include many test cases to be sure your code is working as designed.

        Show
        kwright@metacarta.com Karl Wright added a comment - Hi Ignacio Vera , In Geo3D, there is currently only support for computing the intersection with shapes that implement GeoArea. These only include GeoBBox (2D) and XYZSolid (3D). Currently, GeoPolygon objects do not implement GeoArea. This is by design. There are many cases and corner cases that would need algorithmic development to properly implement polygon intersection. If you need to support this case, feel free to supply a patch where GeoPolygon extends GeoArea. You will need to implement getRelationship() for GeoComplexPolygon, GeoConvexPolygon, GeoConcavePolygon, and GeoCompositePolygon. I would also urge you to include many test cases to be sure your code is working as designed.
        Hide
        ivera Ignacio Vera added a comment -

        Hi Karl Wright,

        I would try to do so. I have already some code that works for simple polygons. Let see if I manage for more complex cases. I am concern with the case of GeoComplexPolygon but I will have a try.

        By the way, there is something in the implementation of getRelationship in GeoRectangle that seems wrong. The method call another method in GeoBaseBox called isShapeInsideBBox. In this methods there is the following call:

        final GeoPoint[] pathPoints = path.getEdgePoints();

        Expecting more than one point returned that is not true as the doc says:

        /**

        • Return a sample point that is on the outside edge/boundary of the shape.
          *
        • @return samples of all edge points from distinct edge sections. Typically one point
        • is returned, but zero or two are also possible.
          */
          public GeoPoint[] getEdgePoints();

        I think that method is not doing what it is expected. Still the overall method getRelationship seems to work.

        Cheers,

        I.

        Show
        ivera Ignacio Vera added a comment - Hi Karl Wright , I would try to do so. I have already some code that works for simple polygons. Let see if I manage for more complex cases. I am concern with the case of GeoComplexPolygon but I will have a try. By the way, there is something in the implementation of getRelationship in GeoRectangle that seems wrong. The method call another method in GeoBaseBox called isShapeInsideBBox. In this methods there is the following call: final GeoPoint[] pathPoints = path.getEdgePoints(); Expecting more than one point returned that is not true as the doc says: /** Return a sample point that is on the outside edge/boundary of the shape. * @return samples of all edge points from distinct edge sections. Typically one point is returned, but zero or two are also possible. */ public GeoPoint[] getEdgePoints(); I think that method is not doing what it is expected. Still the overall method getRelationship seems to work. Cheers, I.
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        Multiple edge points must get returned when there are disconnected sections of the shape, when projected onto the world surface. This happens in a couple of situations, most notably for XYZSolids, which can slice straight through a world and manifest themselves as multiple actual areas.

        There is also no harm in returning more than one point, even for a connected area, except for performance loss.

        Show
        kwright@metacarta.com Karl Wright added a comment - Multiple edge points must get returned when there are disconnected sections of the shape, when projected onto the world surface. This happens in a couple of situations, most notably for XYZSolids, which can slice straight through a world and manifest themselves as multiple actual areas. There is also no harm in returning more than one point, even for a connected area, except for performance loss.
        Hide
        ivera Ignacio Vera added a comment -

        Ok, I understand and I will try to follow similar strategy for polygons when possible.

        Thanks!

        Ignacio

        Show
        ivera Ignacio Vera added a comment - Ok, I understand and I will try to follow similar strategy for polygons when possible. Thanks! Ignacio
        Hide
        ivera Ignacio Vera added a comment -

        Hi Karl Wright,

        Attached is the patches implementing GeoArea for polygons. I provide extended testing,
        Some notes:

        a) I have to modified the method getEdgePoints() from GeoCompositeMembershipShape. It ewas returning only the edge points for the first shape which seems incorrect. My implementation is not very efficient but works.

        b)To be able able to bound the intersection of convex polygons I create the conves counter part. Maybe there is more efficient way of doing that.

        c) Would it be worthy to push the geoArea interface down to GeoShape?

        Cheers,

        I.

        Show
        ivera Ignacio Vera added a comment - Hi Karl Wright , Attached is the patches implementing GeoArea for polygons. I provide extended testing, Some notes: a) I have to modified the method getEdgePoints() from GeoCompositeMembershipShape. It ewas returning only the edge points for the first shape which seems incorrect. My implementation is not very efficient but works. b)To be able able to bound the intersection of convex polygons I create the conves counter part. Maybe there is more efficient way of doing that. c) Would it be worthy to push the geoArea interface down to GeoShape? Cheers, I.
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        Hi Ignacio Vera, we do not want to push GeoArea implementation down to GeoShape, because of shapes that cannot compute intersections readily, e.g. GeoCircle.

        I agree that your implementation of GeoCompositePolygon.getEdgePoints() is correct.

        I'll review your patch when time permits; it will have to wait until tomorrow, most likely.

        Show
        kwright@metacarta.com Karl Wright added a comment - Hi Ignacio Vera , we do not want to push GeoArea implementation down to GeoShape, because of shapes that cannot compute intersections readily, e.g. GeoCircle. I agree that your implementation of GeoCompositePolygon.getEdgePoints() is correct. I'll review your patch when time permits; it will have to wait until tomorrow, most likely.
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        I had a quick look at the diff, and found this issue:

        +  @Override
        +  public int getRelationship(GeoShape shape){
        +    boolean isWithin = false;
        +    boolean isContains = false;
        +    for (int i=0; i<shapes.size(); i++) {
        +      GeoPolygon pol = (GeoPolygon) shapes.get(i);
        +      int relationship = pol.getRelationship(shape);
        +      switch (relationship){
        +        case GeoArea.OVERLAPS: return relationship;
        +        case GeoArea.WITHIN: isWithin=true; break;
        +        case GeoArea.CONTAINS: isContains=true;break;
        +        case GeoArea.DISJOINT: break;
        +      }
        +    }
        

        I note that OVERLAPS subcomponents return OVERLAPS as the result. But this cannot be correct because an OVERLAP between a subcomponent might actually be WITHIN.

        Show
        kwright@metacarta.com Karl Wright added a comment - I had a quick look at the diff, and found this issue: + @Override + public int getRelationship(GeoShape shape){ + boolean isWithin = false ; + boolean isContains = false ; + for ( int i=0; i<shapes.size(); i++) { + GeoPolygon pol = (GeoPolygon) shapes.get(i); + int relationship = pol.getRelationship(shape); + switch (relationship){ + case GeoArea.OVERLAPS: return relationship; + case GeoArea.WITHIN: isWithin= true ; break ; + case GeoArea.CONTAINS: isContains= true ; break ; + case GeoArea.DISJOINT: break ; + } + } I note that OVERLAPS subcomponents return OVERLAPS as the result. But this cannot be correct because an OVERLAP between a subcomponent might actually be WITHIN.
        Hide
        ivera Ignacio Vera added a comment -

        I thought it was enough with skipping the internal edges but I can reproduce the behavior when I have a collection of shapes inside a GeoCompositeMembershipShape.

        The wrong behavior happens because of this call GeoBaseShape.isShapeInsidePolygon(final GeoShape geoShape).

        When checking in a multishape you might get SOME_INSIDE and therefore overlaps.

        The Polygon to be compared should be the composite polygon and not the subcomponent so the methods returns the correct answer ALL_INSIDE.

        What it means is that either GeoConvexPolygon and GeoConcavePolygon should have a reference to the parent composite polygon. I have implemented it and it works but it means that the contructor for those objects need to change. Is that ok?

        Show
        ivera Ignacio Vera added a comment - I thought it was enough with skipping the internal edges but I can reproduce the behavior when I have a collection of shapes inside a GeoCompositeMembershipShape. The wrong behavior happens because of this call GeoBaseShape.isShapeInsidePolygon(final GeoShape geoShape). When checking in a multishape you might get SOME_INSIDE and therefore overlaps. The Polygon to be compared should be the composite polygon and not the subcomponent so the methods returns the correct answer ALL_INSIDE. What it means is that either GeoConvexPolygon and GeoConcavePolygon should have a reference to the parent composite polygon. I have implemented it and it works but it means that the contructor for those objects need to change. Is that ok?
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        Hi Ignacio Vera, I think it is OK to include a reference to the containing GeoPolygon in the constructor – but then you'd also need a pointer to the enclosing GeoPolygon in the constructor for GeoCompositePolygon, because they can be nested (and the factory constructs them in a nested fashion).

        I don't think you'll be able to have GeoCompositePolygon simply call the getRelationship() methods of its child GeoPolygon members. I think you'll need to compute the intersection from first principles in GeoCompositePolygon itself, by asking the child polygons if any of their external edges get intersected etc. That may require the addition of a new method to GeoPolygon, but that's what I think is needed.

        Show
        kwright@metacarta.com Karl Wright added a comment - Hi Ignacio Vera , I think it is OK to include a reference to the containing GeoPolygon in the constructor – but then you'd also need a pointer to the enclosing GeoPolygon in the constructor for GeoCompositePolygon, because they can be nested (and the factory constructs them in a nested fashion). I don't think you'll be able to have GeoCompositePolygon simply call the getRelationship() methods of its child GeoPolygon members. I think you'll need to compute the intersection from first principles in GeoCompositePolygon itself, by asking the child polygons if any of their external edges get intersected etc. That may require the addition of a new method to GeoPolygon, but that's what I think is needed.
        Hide
        ivera Ignacio Vera added a comment -

        Yes, adding a new interface method to the polygon interface does the trick. I have added the method GeoPolygon.intersects(Geshape geoshape) and it is working as expected. Patch attached

        Show
        ivera Ignacio Vera added a comment - Yes, adding a new interface method to the polygon interface does the trick. I have added the method GeoPolygon.intersects(Geshape geoshape) and it is working as expected. Patch attached
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        Ignacio Vera, I had a quick glance at the newer patch. So far, so good, but have a care for spelling and for code style. Have a look here:

        +  private boolean instersectsEdge(GeoShape other,Edge currentEdge,Edge firstEdge){
        +    if (currentEdge == firstEdge){
        +      return false;
        +    }
        +    if (other.intersects(currentEdge.plane,currentEdge.notablePoints,currentEdge.startPlane,currentEdge.endPlane)) {
        +      return  true;
        +    }
        +    if (firstEdge == null){
        +      firstEdge = currentEdge;
        +    }
        +    return instersectsEdge(other,currentEdge.next,firstEdge);
        +  }
        

        First note: "instersectsEdge" is misspelled; let's fix that before proceeding to commit.
        Second, the style guide for Lucene states that there's a space after commas in argument lists for methods.

        Minor details, but important. I'll have a longer look this evening.

        Show
        kwright@metacarta.com Karl Wright added a comment - Ignacio Vera , I had a quick glance at the newer patch. So far, so good, but have a care for spelling and for code style. Have a look here: + private boolean instersectsEdge(GeoShape other,Edge currentEdge,Edge firstEdge){ + if (currentEdge == firstEdge){ + return false ; + } + if (other.intersects(currentEdge.plane,currentEdge.notablePoints,currentEdge.startPlane,currentEdge.endPlane)) { + return true ; + } + if (firstEdge == null ){ + firstEdge = currentEdge; + } + return instersectsEdge(other,currentEdge.next,firstEdge); + } First note: "instersectsEdge" is misspelled; let's fix that before proceeding to commit. Second, the style guide for Lucene states that there's a space after commas in argument lists for methods. Minor details, but important. I'll have a longer look this evening.
        Hide
        ivera Ignacio Vera added a comment -

        Yes, you are right.

        I fixed the spelling mistake and improve coding style.

        Show
        ivera Ignacio Vera added a comment - Yes, you are right. I fixed the spelling mistake and improve coding style.
        Hide
        ivera Ignacio Vera added a comment -

        Move the getRelationship method down to the GeoBasePolygon.

        Show
        ivera Ignacio Vera added a comment - Move the getRelationship method down to the GeoBasePolygon.
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        Hi Ignacio Vera, I had a more detailed look a this patch.

        I note that there is a symmetry difference between GeoConcavePolygon and GeoConvexPolygon that I don't understand. GeoConcavePolygon has the following lazy-init'd member variable:

        +  /** Convex polygon counter part used for bounding intersections. Lazy initialized */
        +  protected GeoPolygon convexPolygon=null;
        +
        

        GeoConvexPolygon has no similar member variable. Can you explain why one has the converse shape, and the other doesn't? What is the code trying to do here?

        I also worry a bit when a direct shape inversion is used, because points that are on a shape edge will be members of both the shape and its inverse. That may not be what you want.

        The tests look good so far, but you really want to consider adding a randomized test as well. Have a look at extending some of the randomized Geo3d testing classes under spatial-extras to include testing intersections against polygons:

        06/29/2017  01:39 PM            10,420 Geo3dRptTest.java
        06/29/2017  01:39 PM             9,563 Geo3dShapeRectRelationTestCase.java
        04/17/2016  03:38 PM             3,418 Geo3dShapeSphereModelRectRelationTest.java
        04/05/2016  06:10 AM             6,144 Geo3dShapeWGS84ModelRectRelationTest.java
        03/31/2016  08:08 PM            10,239 RandomizedShapeTestCase.java
        
        Show
        kwright@metacarta.com Karl Wright added a comment - Hi Ignacio Vera , I had a more detailed look a this patch. I note that there is a symmetry difference between GeoConcavePolygon and GeoConvexPolygon that I don't understand. GeoConcavePolygon has the following lazy-init'd member variable: + /** Convex polygon counter part used for bounding intersections. Lazy initialized */ + protected GeoPolygon convexPolygon= null ; + GeoConvexPolygon has no similar member variable. Can you explain why one has the converse shape, and the other doesn't? What is the code trying to do here? I also worry a bit when a direct shape inversion is used, because points that are on a shape edge will be members of both the shape and its inverse. That may not be what you want. The tests look good so far, but you really want to consider adding a randomized test as well. Have a look at extending some of the randomized Geo3d testing classes under spatial-extras to include testing intersections against polygons: 06/29/2017 01:39 PM 10,420 Geo3dRptTest.java 06/29/2017 01:39 PM 9,563 Geo3dShapeRectRelationTestCase.java 04/17/2016 03:38 PM 3,418 Geo3dShapeSphereModelRectRelationTest.java 04/05/2016 06:10 AM 6,144 Geo3dShapeWGS84ModelRectRelationTest.java 03/31/2016 08:08 PM 10,239 RandomizedShapeTestCase.java
        Hide
        ivera Ignacio Vera added a comment -

        Hi Karl Wright,

        The problem with convex polygons is that the whole plane is within the shape and therefore if you try to bound the intersection with the convex polygon you get OVERLAP when the shape is actually WITHIN. What it holds true is that the bounds for an intersection for convex polygons are the ones defined by the convex counter part. Therefore I need to invert the shape to bound the intersection. The class variable is to cash the object so it is only created once. Does it make sense?

        I will try to implement randomized tests as well but it will take a bit longer because I have never used the framework and I am actually having problems running those tests (environment issues).

        Finally, I want to go back to the idea of moving GeoArea down to GeoShape. If the implementation for polygons is valid, it means that any shape that can implement the new interface method intersects(GeoShape geoShape) can implement GeoArea. You were concerned about circle intersection but I think it is a trivial implementation. We only need to add the interface GeoOutsideDistance to GeoShape which is free as all shapes already implement the interface. Then intersection is trivial by calculating the distance of the shape to the center of the circle. What do you think?

        Cheers,

        Ignacio

        Show
        ivera Ignacio Vera added a comment - Hi Karl Wright , The problem with convex polygons is that the whole plane is within the shape and therefore if you try to bound the intersection with the convex polygon you get OVERLAP when the shape is actually WITHIN. What it holds true is that the bounds for an intersection for convex polygons are the ones defined by the convex counter part. Therefore I need to invert the shape to bound the intersection. The class variable is to cash the object so it is only created once. Does it make sense? I will try to implement randomized tests as well but it will take a bit longer because I have never used the framework and I am actually having problems running those tests (environment issues). Finally, I want to go back to the idea of moving GeoArea down to GeoShape. If the implementation for polygons is valid, it means that any shape that can implement the new interface method intersects(GeoShape geoShape) can implement GeoArea. You were concerned about circle intersection but I think it is a trivial implementation. We only need to add the interface GeoOutsideDistance to GeoShape which is free as all shapes already implement the interface. Then intersection is trivial by calculating the distance of the shape to the center of the circle. What do you think? Cheers, Ignacio
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        Ignacio Vera, requiring GeoArea implementation for all GeoShapes very much complicates the implementation of new GeoShapes, and I would be very much against it. You are welcome to try to implement GeoArea for GeoCircles but remember that your distance computation does not work in WGS84, only in SPHERE. Complex shapes like GeoPaths are not only messy to make work generally, but they also have even messier considerations when you use WGS84. Polygons in WGS84 aren't much harder than polygons in SPHERE which is why I thought that was doable, but after that point it gets much tougher. The "GeoCircle" is by definition the intersection of a specific plane with the unit ellipsoid, so it's in fact an ellipse and distance calculations are exorbitantly expensive.

        One of the reasons I am absolutely certain we need to include GeoPolygons in the randomized testing is because, for the geo3d world, such testing is essential to be sure you've really covered all the corner cases and have no numeric precision problems. So this is something we need to tackle I'm afraid.

        If your concern is that you want new shapes to be able to support your way of computing intersection, let me point out that any shape where the edge planes go through the ellipsoid center is likely to be represented as a polygon, so you've got that well covered. It's the shapes that don't have that characteristic that I don't think are straightforward to implement. So for that reason I'm OK with the way it's set up now.

        Thanks!

        Show
        kwright@metacarta.com Karl Wright added a comment - Ignacio Vera , requiring GeoArea implementation for all GeoShapes very much complicates the implementation of new GeoShapes, and I would be very much against it. You are welcome to try to implement GeoArea for GeoCircles but remember that your distance computation does not work in WGS84, only in SPHERE. Complex shapes like GeoPaths are not only messy to make work generally, but they also have even messier considerations when you use WGS84. Polygons in WGS84 aren't much harder than polygons in SPHERE which is why I thought that was doable, but after that point it gets much tougher. The "GeoCircle" is by definition the intersection of a specific plane with the unit ellipsoid, so it's in fact an ellipse and distance calculations are exorbitantly expensive. One of the reasons I am absolutely certain we need to include GeoPolygons in the randomized testing is because, for the geo3d world, such testing is essential to be sure you've really covered all the corner cases and have no numeric precision problems. So this is something we need to tackle I'm afraid. If your concern is that you want new shapes to be able to support your way of computing intersection, let me point out that any shape where the edge planes go through the ellipsoid center is likely to be represented as a polygon, so you've got that well covered. It's the shapes that don't have that characteristic that I don't think are straightforward to implement. So for that reason I'm OK with the way it's set up now. Thanks!
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        The problem with convex polygons is that the whole plane is within the shape and therefore if you try to bound the intersection with the convex polygon you get OVERLAP when the shape is actually WITHIN. What it holds true is that the bounds for an intersection for convex polygons are the ones defined by the convex counter part. Therefore I need to invert the shape to bound the intersection. The class variable is to cash the object so it is only created once. Does it make sense?

        Yes, I understand the limitations. I'm not, however, thrilled with the lazy-init part of this since it gives us some thread safety concerns, and the "inverse shape" way of doing things has problems as I described earlier. If all you are trying to do is model the bounds for each edge plane, you're probably better off, and safer too, computing and saving those bounds when the polygon is constructed. You'll especially need to worry about almost-coplanar adjacent polygon edges; it may be better to construct a pair of perpendicular planes for each edge if you need a good bound. Or, you can use the adjoining edges but just invert their sidedness; there's a SidedPlane constructor that does that. Let me look carefully at the code for both kinds of polygon and give you my recommendation.

        The above concerns are exactly why randomized tests are so valuable, by the way, and why they really need to be there before we can have assurance that your implementation is not going to generate a slew of bug reports when it hits the real world.

        The randomized tests I pointed you at earlier construct mainly GeoBBox shapes to intersect against. You'd want almost the same test but with random (but regular and not self-overlapping) GeoPolygons. You'll need to put some thought into how to construct a random, non-degenerate GeoConvexPolygon or GeoConcavePolygon, especially with holes. There are IllegalArgumentException conditions thrown by the constructors when shapes are found to be illegal for some reason, but I believe we removed some of these in the polygon constructors because of their cost. Other randomized tests that construct GeoPolygons use a center point, and walk around the center point N times with a random angle each time and a random arc distance; that guarantees non-self-intersecting but doesn't prevent concavity/convexity. You could use the GeoPolygonFactory method, though, to create a composite polygon from this consisting of multiple child shapes and that is guaranteed to work and be legal. So I encourage you to look at how polygon construction is done in the existing tests.

        Thanks!

        Show
        kwright@metacarta.com Karl Wright added a comment - The problem with convex polygons is that the whole plane is within the shape and therefore if you try to bound the intersection with the convex polygon you get OVERLAP when the shape is actually WITHIN. What it holds true is that the bounds for an intersection for convex polygons are the ones defined by the convex counter part. Therefore I need to invert the shape to bound the intersection. The class variable is to cash the object so it is only created once. Does it make sense? Yes, I understand the limitations. I'm not, however, thrilled with the lazy-init part of this since it gives us some thread safety concerns, and the "inverse shape" way of doing things has problems as I described earlier. If all you are trying to do is model the bounds for each edge plane, you're probably better off, and safer too, computing and saving those bounds when the polygon is constructed. You'll especially need to worry about almost-coplanar adjacent polygon edges; it may be better to construct a pair of perpendicular planes for each edge if you need a good bound. Or, you can use the adjoining edges but just invert their sidedness; there's a SidedPlane constructor that does that. Let me look carefully at the code for both kinds of polygon and give you my recommendation. The above concerns are exactly why randomized tests are so valuable, by the way, and why they really need to be there before we can have assurance that your implementation is not going to generate a slew of bug reports when it hits the real world. The randomized tests I pointed you at earlier construct mainly GeoBBox shapes to intersect against. You'd want almost the same test but with random (but regular and not self-overlapping) GeoPolygons. You'll need to put some thought into how to construct a random, non-degenerate GeoConvexPolygon or GeoConcavePolygon, especially with holes. There are IllegalArgumentException conditions thrown by the constructors when shapes are found to be illegal for some reason, but I believe we removed some of these in the polygon constructors because of their cost. Other randomized tests that construct GeoPolygons use a center point, and walk around the center point N times with a random angle each time and a random arc distance; that guarantees non-self-intersecting but doesn't prevent concavity/convexity. You could use the GeoPolygonFactory method, though, to create a composite polygon from this consisting of multiple child shapes and that is guaranteed to work and be legal. So I encourage you to look at how polygon construction is done in the existing tests. Thanks!
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        Hi Ignacio Vera, for both GeoConcavePolygon and GeoConvexPolygon, there is a member variable already constructed as follows:

          /** A bounds object for each sided plane */
          protected Map<SidedPlane, Membership> eitherBounds = null;
        

        This map can be used to look up the bounds, as a single Membership object, for every edge plane in the polygon. I suggest you use this rather than some other way of doing it.

        Thanks!

        Show
        kwright@metacarta.com Karl Wright added a comment - Hi Ignacio Vera , for both GeoConcavePolygon and GeoConvexPolygon, there is a member variable already constructed as follows: /** A bounds object for each sided plane */ protected Map<SidedPlane, Membership> eitherBounds = null ; This map can be used to look up the bounds, as a single Membership object, for every edge plane in the polygon. I suggest you use this rather than some other way of doing it. Thanks!
        Hide
        ivera Ignacio Vera added a comment -

        Ok, I did not know that distance calculation is only support for unit sphere. That is my use case so I guess I am bias. It is totally fine to leave things then as they are.

        Thanks for your suggestions, I will try to put everything together in the next week or so.

        Cheers,

        Ignacio

        Show
        ivera Ignacio Vera added a comment - Ok, I did not know that distance calculation is only support for unit sphere. That is my use case so I guess I am bias. It is totally fine to leave things then as they are. Thanks for your suggestions, I will try to put everything together in the next week or so. Cheers, Ignacio
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        Ignacio Vera, arcDistance is defined well for ellipsoids but surfaceDistance requires an iterative convergent loop. Also, the GeoCircle is in fact a single plane which slices through the ellipsoid forming an ellipse. Since surfaceDistance is prohibitively expensive to calculate for all but a sphere, an approximation is required either way, and the GeoCircle is fitted specially to the ellipsoid in order to be a good approximation for WGS84.

        Show
        kwright@metacarta.com Karl Wright added a comment - Ignacio Vera , arcDistance is defined well for ellipsoids but surfaceDistance requires an iterative convergent loop. Also, the GeoCircle is in fact a single plane which slices through the ellipsoid forming an ellipse. Since surfaceDistance is prohibitively expensive to calculate for all but a sphere, an approximation is required either way, and the GeoCircle is fitted specially to the ellipsoid in order to be a good approximation for WGS84.
        Hide
        ivera Ignacio Vera added a comment - - edited

        My qiestion is, is there any issue to implement intersects in the circle as:

        @Override
        public boolean intersects(GeoShape shape) {
        if (circlePlane == null)

        { return false; }

        return shape.intersects(circlePlane, circlePoints);
        }

        On another topic, I am having problems with GeoComplexPolygon. It seems they way I have implemented and the more efficient way of intersects with plane on the class give different results. I tried to implement an IntersectorShapeIterator but some of my test were failing. I need to have a closer look into that.

        Show
        ivera Ignacio Vera added a comment - - edited My qiestion is, is there any issue to implement intersects in the circle as: @Override public boolean intersects(GeoShape shape) { if (circlePlane == null) { return false; } return shape.intersects(circlePlane, circlePoints); } On another topic, I am having problems with GeoComplexPolygon. It seems they way I have implemented and the more efficient way of intersects with plane on the class give different results. I tried to implement an IntersectorShapeIterator but some of my test were failing. I need to have a closer look into that.
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        My qiestion is, is there any issue to implement intersects in the circle as:

        Ignacio Vera Are you using "p" when you mean "circlePlane"? This may work for circles, but it would need to be thoroughly tested.

        On another topic, I am having problems with GeoComplexPolygon. It seems they way I have implemented and the more efficient way of intersects with plane on the class give different results. I tried to implement an IntersectorShapeIterator but some of my test were failing. I need to have a closer look into that.

        I am not sure quite what you mean here? Can you give me a snippet of code?

        Show
        kwright@metacarta.com Karl Wright added a comment - My qiestion is, is there any issue to implement intersects in the circle as: Ignacio Vera Are you using "p" when you mean "circlePlane"? This may work for circles, but it would need to be thoroughly tested. On another topic, I am having problems with GeoComplexPolygon. It seems they way I have implemented and the more efficient way of intersects with plane on the class give different results. I tried to implement an IntersectorShapeIterator but some of my test were failing. I need to have a closer look into that. I am not sure quite what you mean here? Can you give me a snippet of code?
        Hide
        ivera Ignacio Vera added a comment - - edited

        I corrected the code above.

        I found the issue, it is in line 496 of GeoComplexPolygon:

        if (right != null && minValue >= low && right.traverse(edgeIterator, minValue, maxValue) == false) {

        It seems that the condition minValue >= low prevents to look for necessary planes. If a change the condition to maxValue >= low my test run smooth but I don't know if it is correct. What it is sure is that the condition is not right. Any ideas?
        Show
        ivera Ignacio Vera added a comment - - edited I corrected the code above. I found the issue, it is in line 496 of GeoComplexPolygon: if (right != null && minValue >= low && right.traverse(edgeIterator, minValue, maxValue) == false) { It seems that the condition minValue >= low prevents to look for necessary planes. If a change the condition to maxValue >= low my test run smooth but I don't know if it is correct. What it is sure is that the condition is not right. Any ideas?
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        Hi Ignacio Vera, the GeoComplexPolygon implementation of Node was modeled directly on Robert Muir's implementation of 2D polygons. The code is very complex and very sensitive to changes, so before we change anything we must confirm that I made an error in revising the original code for the 3D case.

        Show
        kwright@metacarta.com Karl Wright added a comment - Hi Ignacio Vera , the GeoComplexPolygon implementation of Node was modeled directly on Robert Muir's implementation of 2D polygons. The code is very complex and very sensitive to changes, so before we change anything we must confirm that I made an error in revising the original code for the 3D case.
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        Here is the current 2D polygon code. It has changed somewhat since I originally used it as a baseline.

          /** 
           * Returns true if the point is contained within this polygon.
           * <p>
           * See <a href="https://www.ecse.rpi.edu/~wrf/Research/Short_Notes/pnpoly.html">
           * https://www.ecse.rpi.edu/~wrf/Research/Short_Notes/pnpoly.html</a> for more information.
           */
          public boolean contains(double latitude, double longitude) {
            if (latitude <= maxY && longitude <= maxX) {
              if (componentContains(latitude, longitude)) {
                return true;
              }
              if (left != null) {
                if (left.contains(latitude, longitude)) {
                  return true;
                }
              }
              if (right != null && ((splitX == false && latitude >= minLat) || (splitX && longitude >= minLon))) {
                if (right.contains(latitude, longitude)) {
                  return true;
                }
              }
            }
            return false;
          }
          
          /** Returns true if the point is contained within this polygon component. */
          private boolean componentContains(double latitude, double longitude) {
            // check bounding box
            if (latitude < minLat || latitude > maxLat || longitude < minLon || longitude > maxLon) {
              return false;
            }
            
            if (tree.contains(latitude, longitude)) {
              if (holes != null && holes.contains(latitude, longitude)) {
                return false;
              }
              return true;
            }
            
            return false;
          }
        
        Show
        kwright@metacarta.com Karl Wright added a comment - Here is the current 2D polygon code. It has changed somewhat since I originally used it as a baseline. /** * Returns true if the point is contained within this polygon. * <p> * See <a href= "https: //www.ecse.rpi.edu/~wrf/Research/Short_Notes/pnpoly.html" > * https: //www.ecse.rpi.edu/~wrf/Research/Short_Notes/pnpoly.html</a> for more information. */ public boolean contains( double latitude, double longitude) { if (latitude <= maxY && longitude <= maxX) { if (componentContains(latitude, longitude)) { return true ; } if (left != null ) { if (left.contains(latitude, longitude)) { return true ; } } if (right != null && ((splitX == false && latitude >= minLat) || (splitX && longitude >= minLon))) { if (right.contains(latitude, longitude)) { return true ; } } } return false ; } /** Returns true if the point is contained within this polygon component. */ private boolean componentContains( double latitude, double longitude) { // check bounding box if (latitude < minLat || latitude > maxLat || longitude < minLon || longitude > maxLon) { return false ; } if (tree.contains(latitude, longitude)) { if (holes != null && holes.contains(latitude, longitude)) { return false ; } return true ; } return false ; }
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        The 2D tree creation code now looks like this:

          /** 
           * Creates an edge interval tree from a set of polygon vertices.
           * @return root node of the tree.
           */
          private static Edge createTree(double polyLats[], double polyLons[]) {
            Edge edges[] = new Edge[polyLats.length - 1];
            for (int i = 1; i < polyLats.length; i++) {
              double lat1 = polyLats[i-1];
              double lon1 = polyLons[i-1];
              double lat2 = polyLats[i];
              double lon2 = polyLons[i];
              edges[i - 1] = new Edge(lat1, lon1, lat2, lon2, Math.min(lat1, lat2), Math.max(lat1, lat2));
            }
            // sort the edges then build a balanced tree from them
            Arrays.sort(edges, (left, right) -> {
              int ret = Double.compare(left.low, right.low);
              if (ret == 0) {
                ret = Double.compare(left.max, right.max);
              }
              return ret;
            });
            return createTree(edges, 0, edges.length - 1);
          }
        
          /** Creates tree from sorted edges (with range low and high inclusive) */
          private static Edge createTree(Edge edges[], int low, int high) {
            if (low > high) {
              return null;
            }
            // add midpoint
            int mid = (low + high) >>> 1;
            Edge newNode = edges[mid];
            // add children
            newNode.left = createTree(edges, low, mid - 1);
            newNode.right = createTree(edges, mid + 1, high);
            // pull up max values to this node
            if (newNode.left != null) {
              newNode.max = Math.max(newNode.max, newNode.left.max);
            }
            if (newNode.right != null) {
              newNode.max = Math.max(newNode.max, newNode.right.max);
            }
            return newNode;
          }
        

        If we adopt the new membership code we would need to adapt the new tree construction code as well.

        Show
        kwright@metacarta.com Karl Wright added a comment - The 2D tree creation code now looks like this: /** * Creates an edge interval tree from a set of polygon vertices. * @ return root node of the tree. */ private static Edge createTree( double polyLats[], double polyLons[]) { Edge edges[] = new Edge[polyLats.length - 1]; for ( int i = 1; i < polyLats.length; i++) { double lat1 = polyLats[i-1]; double lon1 = polyLons[i-1]; double lat2 = polyLats[i]; double lon2 = polyLons[i]; edges[i - 1] = new Edge(lat1, lon1, lat2, lon2, Math .min(lat1, lat2), Math .max(lat1, lat2)); } // sort the edges then build a balanced tree from them Arrays.sort(edges, (left, right) -> { int ret = Double .compare(left.low, right.low); if (ret == 0) { ret = Double .compare(left.max, right.max); } return ret; }); return createTree(edges, 0, edges.length - 1); } /** Creates tree from sorted edges (with range low and high inclusive) */ private static Edge createTree(Edge edges[], int low, int high) { if (low > high) { return null ; } // add midpoint int mid = (low + high) >>> 1; Edge newNode = edges[mid]; // add children newNode.left = createTree(edges, low, mid - 1); newNode.right = createTree(edges, mid + 1, high); // pull up max values to this node if (newNode.left != null ) { newNode.max = Math .max(newNode.max, newNode.left.max); } if (newNode.right != null ) { newNode.max = Math .max(newNode.max, newNode.right.max); } return newNode; } If we adopt the new membership code we would need to adapt the new tree construction code as well.
        Hide
        ivera Ignacio Vera added a comment -

        My feel is that there is a min value missing:

        • high: higher value of the node
        • low: lower value ofthe node
        • max: maximum value of the tree
        • min: (missing) minimum value of the tree.

        The condition you want to add is: minValue>=min insterad of minValue>=low ... then you discard edges out of scope.

        Show
        ivera Ignacio Vera added a comment - My feel is that there is a min value missing: high: higher value of the node low: lower value ofthe node max: maximum value of the tree min: (missing) minimum value of the tree. The condition you want to add is: minValue>=min insterad of minValue>=low ... then you discard edges out of scope.
        Hide
        kwright@metacarta.com Karl Wright added a comment - - edited

        Ah, I included the wrong code. The "crosses" detection uses this logic:

                if (left != null) {
                  if (left.crosses(minLat, maxLat, minLon, maxLon)) {
                    return true;
                  }
                }
                
                if (right != null && maxLat >= low) {
                  if (right.crosses(minLat, maxLat, minLon, maxLon)) {
                    return true;
                  }
                }
        

        So, you are correct; the code should read "maxValue >= low".

        Show
        kwright@metacarta.com Karl Wright added a comment - - edited Ah, I included the wrong code. The "crosses" detection uses this logic: if (left != null ) { if (left.crosses(minLat, maxLat, minLon, maxLon)) { return true ; } } if (right != null && maxLat >= low) { if (right.crosses(minLat, maxLat, minLon, maxLon)) { return true ; } } So, you are correct; the code should read "maxValue >= low".
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 2eba4cebc5fa963dc90cf31e8b10801c0161fc55 in lucene-solr's branch refs/heads/master from Karl Wright
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=2eba4ce ]

        LUCENE-7906: Logic fix for crosses condition

        Show
        jira-bot ASF subversion and git services added a comment - Commit 2eba4cebc5fa963dc90cf31e8b10801c0161fc55 in lucene-solr's branch refs/heads/master from Karl Wright [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=2eba4ce ] LUCENE-7906 : Logic fix for crosses condition
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 5aa71427f2e4b22da22836e923fb52361f0d1721 in lucene-solr's branch refs/heads/branch_6x from Karl Wright
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=5aa7142 ]

        LUCENE-7906: Logic fix for crosses condition

        Show
        jira-bot ASF subversion and git services added a comment - Commit 5aa71427f2e4b22da22836e923fb52361f0d1721 in lucene-solr's branch refs/heads/branch_6x from Karl Wright [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=5aa7142 ] LUCENE-7906 : Logic fix for crosses condition
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 5d0e58fd7ac7f9cbe9f520f848b7fef847869130 in lucene-solr's branch refs/heads/branch_7x from Karl Wright
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=5d0e58f ]

        LUCENE-7906: Logic fix for crosses condition

        Show
        jira-bot ASF subversion and git services added a comment - Commit 5d0e58fd7ac7f9cbe9f520f848b7fef847869130 in lucene-solr's branch refs/heads/branch_7x from Karl Wright [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=5d0e58f ] LUCENE-7906 : Logic fix for crosses condition
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        Ignacio Vera, I have pushed the change to GeoComplexPolygon that you suggested to all pertinent branches. Hopefully this will no longer block development for you.

        Show
        kwright@metacarta.com Karl Wright added a comment - Ignacio Vera , I have pushed the change to GeoComplexPolygon that you suggested to all pertinent branches. Hopefully this will no longer block development for you.
        Hide
        ivera Ignacio Vera added a comment -

        Great! Shall I add it to the patch?

        I have the random test almost ready except for polygon with holes. I am concern because sometimes get a bit stuck when trying to build shapes with very specific characteristics. I will send you a new patch soon.

        Show
        ivera Ignacio Vera added a comment - Great! Shall I add it to the patch? I have the random test almost ready except for polygon with holes. I am concern because sometimes get a bit stuck when trying to build shapes with very specific characteristics. I will send you a new patch soon.
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        Ignacio Vera The patch should not contain the GeoComplexPolygon fix, since that has been already committed. You may want to git pull before generating the new patch?

        Show
        kwright@metacarta.com Karl Wright added a comment - Ignacio Vera The patch should not contain the GeoComplexPolygon fix, since that has been already committed. You may want to git pull before generating the new patch?
        Hide
        ivera Ignacio Vera added a comment -

        Hi Karl Wright,

        I have attached a new path withrandom test. I have actually created a randomGeoShapeGenerator that allows me to generate Random shapes with known spatial properties. All tests seems ok.

        I have tried to push down GeoArea down to GeoShape and I didn't get any surpises and it can be easily test with small modification in the new random shape generator. But (there is always a but), the implementation is currently not so clean because the class GeoCompositeMembershipShape does not extends any of the base classes.

        Cheers,

        I.

        Show
        ivera Ignacio Vera added a comment - Hi Karl Wright , I have attached a new path withrandom test. I have actually created a randomGeoShapeGenerator that allows me to generate Random shapes with known spatial properties. All tests seems ok. I have tried to push down GeoArea down to GeoShape and I didn't get any surpises and it can be easily test with small modification in the new random shape generator. But (there is always a but), the implementation is currently not so clean because the class GeoCompositeMembershipShape does not extends any of the base classes. Cheers, I.
        Hide
        kwright@metacarta.com Karl Wright added a comment - - edited

        Ignacio Vera, thanks for the tests! I've had a look, and I think this is in good enough shape to commit.

        The only comments I have are:

        (1) If there's any thought that the "intersects" method being added to GeoPolygon might need to be added to other shapes, e.g. GeoCircle or GeoPath, then we should think about the interface structure more, as you have hinted. There are a number of possibilities here; I'm interested mainly in permitting shapes that do not understand how to intersect with other shapes. Maybe we want to insert another level in the interface hierarchy that would allow this characteristic to be separable, e.g. GeoIntersectableShape?

        (2) The randomized tests seem to concentrate on PlanetModel.SPHERE. Are there any WGS84 tests? That's where a lot of issues might arise.

        Show
        kwright@metacarta.com Karl Wright added a comment - - edited Ignacio Vera , thanks for the tests! I've had a look, and I think this is in good enough shape to commit. The only comments I have are: (1) If there's any thought that the "intersects" method being added to GeoPolygon might need to be added to other shapes, e.g. GeoCircle or GeoPath, then we should think about the interface structure more, as you have hinted. There are a number of possibilities here; I'm interested mainly in permitting shapes that do not understand how to intersect with other shapes. Maybe we want to insert another level in the interface hierarchy that would allow this characteristic to be separable, e.g. GeoIntersectableShape? (2) The randomized tests seem to concentrate on PlanetModel.SPHERE. Are there any WGS84 tests? That's where a lot of issues might arise.
        Hide
        ivera Ignacio Vera added a comment -

        No, The current patch is very similar to the last one only adding GeoArea to GeoPolygons but with the radomShapeGenerator tests. I am looking forward to hearing what you want to propose.

        Show
        ivera Ignacio Vera added a comment - No, The current patch is very similar to the last one only adding GeoArea to GeoPolygons but with the radomShapeGenerator tests. I am looking forward to hearing what you want to propose.
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        Ignacio Vera, here is my hierarchy change proposal.

        The current main hierarchy is:

        GeoDistanceShape -> GeoMembershipShape -> GeoShape

        The various Geo shapes all implement either GeoDistanceShape or GeoMembershipShape.

        I would propose that we expand the hierarchy in this way:

        GeoDistanceShape -> GeoAreaShape -> GeoMembershipShape -> GeoShape

        The "intersects" method would be defined in GeoAreaShape, and it would also include "implements GeoArea". The base class hierarchy would, likewise, be extended, and a new GeoBaseAreaShape base class added.

        What do you think?

        Show
        kwright@metacarta.com Karl Wright added a comment - Ignacio Vera , here is my hierarchy change proposal. The current main hierarchy is: GeoDistanceShape -> GeoMembershipShape -> GeoShape The various Geo shapes all implement either GeoDistanceShape or GeoMembershipShape. I would propose that we expand the hierarchy in this way: GeoDistanceShape -> GeoAreaShape -> GeoMembershipShape -> GeoShape The "intersects" method would be defined in GeoAreaShape, and it would also include "implements GeoArea". The base class hierarchy would, likewise, be extended, and a new GeoBaseAreaShape base class added. What do you think?
        Hide
        ivera Ignacio Vera added a comment -

        (1) I propose is to create a new interface that implements GeoShape and GeoArea and adds the insersects method. I would prefer to call the interface GeoAreaShape.

        (2) No, the randomized tests use a random planetmodel calling the method randomPlanetModel(). Note I kept the orginal hardcoded test I developed at the begining, which are focused in the SPHERE.

        Show
        ivera Ignacio Vera added a comment - (1) I propose is to create a new interface that implements GeoShape and GeoArea and adds the insersects method. I would prefer to call the interface GeoAreaShape. (2) No, the randomized tests use a random planetmodel calling the method randomPlanetModel(). Note I kept the orginal hardcoded test I developed at the begining, which are focused in the SPHERE.
        Hide
        ivera Ignacio Vera added a comment -

        We clearly agree on the name. 100% agree, I will work on that!

        Show
        ivera Ignacio Vera added a comment - We clearly agree on the name. 100% agree, I will work on that!
        Hide
        ivera Ignacio Vera added a comment -

        Hi Karl Wright,

        Attached is a new Patch with the new proposed interface GeoAreaShape. It was trivial to add this new interface for existing Shapes.

        I created a new composite for GeoAreaShape and use it as the base class for GeoCompositePolygon.

        Show
        ivera Ignacio Vera added a comment - Hi Karl Wright , Attached is a new Patch with the new proposed interface GeoAreaShape. It was trivial to add this new interface for existing Shapes. I created a new composite for GeoAreaShape and use it as the base class for GeoCompositePolygon.
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        Hi Ignacio Vera, I tried folding your patch into my workarea and running the standard "ant test" on it.

        (1) I got 20 warnings that look like we should fix them:

            [javac]                                     ^
            [javac] C:\wipgit\lucene4\lucene-solr\lucene\spatial3d\src\test\org\apache\lucene\spatial3d\geom\RandomGeoShapeRelationshipTest.java:131: warning: auxiliary class Constraints in C:\wipgit\lucene4\lucene-solr\lucene\spatial3d\src\test\org\apache\lucene\spatial3d\geom\RandomGeoShapeGenerator.java should not be accessed from outside its own source file
            [javac]         constraints = new Constraints();
            [javac]                           ^
        

        The new test RandomGeoShapeRelationshipTest takes too long to run (5 minutes) for normal consumption. Usually tests have a certain relatively small number of iterations but we run them multiple times through the process known as "beasting". For example:

        ant beast -Dbeast.iters=100 -Dtestcase=TestGeo3DPoint -Dtestmethod=testRandomMedium -Dtests.dups=6 -Dtests.iters=10
        

        Can you reduce the number of internal iterations that test does? Or better yet, point me at where I'd do that? Thanks!

        Show
        kwright@metacarta.com Karl Wright added a comment - Hi Ignacio Vera , I tried folding your patch into my workarea and running the standard "ant test" on it. (1) I got 20 warnings that look like we should fix them: [javac] ^ [javac] C:\wipgit\lucene4\lucene-solr\lucene\spatial3d\src\test\org\apache\lucene\spatial3d\geom\RandomGeoShapeRelationshipTest.java:131: warning: auxiliary class Constraints in C:\wipgit\lucene4\lucene-solr\lucene\spatial3d\src\test\org\apache\lucene\spatial3d\geom\RandomGeoShapeGenerator.java should not be accessed from outside its own source file [javac] constraints = new Constraints(); [javac] ^ The new test RandomGeoShapeRelationshipTest takes too long to run (5 minutes) for normal consumption. Usually tests have a certain relatively small number of iterations but we run them multiple times through the process known as "beasting". For example: ant beast -Dbeast.iters=100 -Dtestcase=TestGeo3DPoint -Dtestmethod=testRandomMedium -Dtests.dups=6 -Dtests.iters=10 Can you reduce the number of internal iterations that test does? Or better yet, point me at where I'd do that? Thanks!
        Hide
        ivera Ignacio Vera added a comment -

        Attached a new version. The compiler warnings should be gone now.

        I lower the iterations so the complete suit typically runs in ~30 sec. The slower test in the new suite is the contains because sometimes it takes a few seconds to find the right shape.

        you can control iteration under the class RandomGeoShapeRelationshipTest. At the moment is set to 5 except for contains which is not repeated.

        Show
        ivera Ignacio Vera added a comment - Attached a new version. The compiler warnings should be gone now. I lower the iterations so the complete suit typically runs in ~30 sec. The slower test in the new suite is the contains because sometimes it takes a few seconds to find the right shape. you can control iteration under the class RandomGeoShapeRelationshipTest. At the moment is set to 5 except for contains which is not repeated.
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        I committed this to master, branch_6x, and branch_7x.

        Ignacio Vera, I highly recommend "beasting" the new tests to see if we get anything out of them under more extreme conditions. If very few iterations require a lot of time to come up with good shapes, it might worth reconsidering shape construction in the tests so that it is less likely to arrive at crazy shapes that need to be discarded. We want to achieve confidence that the tests are in fact heavily exercising the logic you've written.

        Thanks again for the contribution! I will be looking forward to using this code myself in the next few months, depending on how things go.

        Show
        kwright@metacarta.com Karl Wright added a comment - I committed this to master, branch_6x, and branch_7x. Ignacio Vera , I highly recommend "beasting" the new tests to see if we get anything out of them under more extreme conditions. If very few iterations require a lot of time to come up with good shapes, it might worth reconsidering shape construction in the tests so that it is less likely to arrive at crazy shapes that need to be discarded. We want to achieve confidence that the tests are in fact heavily exercising the logic you've written. Thanks again for the contribution! I will be looking forward to using this code myself in the next few months, depending on how things go.
        Hide
        ivera Ignacio Vera added a comment -

        Karl Wright, thanks for committing this new feature, I hope it will prove very useful as well for my use case. I am already using them in LUCENE with very promising results.

        The big offender is the CONTAINS test as finding the right shape can be difficult because there is too much freedom. I have considered removing it all together as the feature is tested in the WITHIN test (being the opposite case). Then the tests converge quite fast.

        Show
        ivera Ignacio Vera added a comment - Karl Wright , thanks for committing this new feature, I hope it will prove very useful as well for my use case. I am already using them in LUCENE with very promising results. The big offender is the CONTAINS test as finding the right shape can be difficult because there is too much freedom. I have considered removing it all together as the feature is tested in the WITHIN test (being the opposite case). Then the tests converge quite fast.
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        FWIW, the commit messages for this ticket describe it as LUCENE-7606, not 7906. My apologies!!

        Show
        kwright@metacarta.com Karl Wright added a comment - FWIW, the commit messages for this ticket describe it as LUCENE-7606 , not 7906. My apologies!!
        Hide
        cpoerschke Christine Poerschke added a comment -

        The https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=39d6be4 commit for this ticket appears to have broken ant precommit - specifically saying Rat problems were found! re: missing Apache License header(s).

        Show
        cpoerschke Christine Poerschke added a comment - The https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=39d6be4 commit for this ticket appears to have broken ant precommit - specifically saying Rat problems were found! re: missing Apache License header(s).
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        Christine Poerschke, "ant precommit" for me blows up on the solr dev guide. I've been blocked from using it for days. I opened a discussion thread in the dev list.

        Perhaps you know why this is happening for me?

        Show
        kwright@metacarta.com Karl Wright added a comment - Christine Poerschke , "ant precommit" for me blows up on the solr dev guide. I've been blocked from using it for days. I opened a discussion thread in the dev list. Perhaps you know why this is happening for me?
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit a381bdbaaab60c9d53a574ba1c8a4b4815b51b85 in lucene-solr's branch refs/heads/master from Karl Wright
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=a381bdb ]

        LUCENE-7906: Fix ant precommit issue with test license header.

        Show
        jira-bot ASF subversion and git services added a comment - Commit a381bdbaaab60c9d53a574ba1c8a4b4815b51b85 in lucene-solr's branch refs/heads/master from Karl Wright [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=a381bdb ] LUCENE-7906 : Fix ant precommit issue with test license header.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit e9ce58b20e182361790edf79a44c38e006395c38 in lucene-solr's branch refs/heads/branch_6x from Karl Wright
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=e9ce58b ]

        LUCENE-7906: Fix ant precommit issue with test license header.

        Show
        jira-bot ASF subversion and git services added a comment - Commit e9ce58b20e182361790edf79a44c38e006395c38 in lucene-solr's branch refs/heads/branch_6x from Karl Wright [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=e9ce58b ] LUCENE-7906 : Fix ant precommit issue with test license header.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 934db4a9c42af1186617afead7edb8dcebcb18eb in lucene-solr's branch refs/heads/branch_7x from Karl Wright
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=934db4a ]

        LUCENE-7906: Fix ant precommit issue with test license header.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 934db4a9c42af1186617afead7edb8dcebcb18eb in lucene-solr's branch refs/heads/branch_7x from Karl Wright [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=934db4a ] LUCENE-7906 : Fix ant precommit issue with test license header.
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        Ignacio Vera, I'm getting an "ant documentation-lint" error for the new class GeoCompositeAreaShape; says there's missing javadoc. I can't find anything obviously wrong with it however.

        Show
        kwright@metacarta.com Karl Wright added a comment - Ignacio Vera , I'm getting an "ant documentation-lint" error for the new class GeoCompositeAreaShape; says there's missing javadoc. I can't find anything obviously wrong with it however.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit c6ae0496627236b0108941e35cb4646935ad53c3 in lucene-solr's branch refs/heads/master from Karl Wright
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=c6ae049 ]

        LUCENE-7906: Add missing override, fixing javadoc

        Show
        jira-bot ASF subversion and git services added a comment - Commit c6ae0496627236b0108941e35cb4646935ad53c3 in lucene-solr's branch refs/heads/master from Karl Wright [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=c6ae049 ] LUCENE-7906 : Add missing override, fixing javadoc
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 4a882739592aea109503c8e62c5730cd9b6ea64b in lucene-solr's branch refs/heads/branch_6x from Karl Wright
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=4a88273 ]

        LUCENE-7906: Add missing override, fixing javadoc

        Show
        jira-bot ASF subversion and git services added a comment - Commit 4a882739592aea109503c8e62c5730cd9b6ea64b in lucene-solr's branch refs/heads/branch_6x from Karl Wright [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=4a88273 ] LUCENE-7906 : Add missing override, fixing javadoc
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 893ea166ef286a7118c3ac5ffc90f27e5b94789d in lucene-solr's branch refs/heads/branch_7x from Karl Wright
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=893ea16 ]

        LUCENE-7906: Add missing override, fixing javadoc

        Show
        jira-bot ASF subversion and git services added a comment - Commit 893ea166ef286a7118c3ac5ffc90f27e5b94789d in lucene-solr's branch refs/heads/branch_7x from Karl Wright [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=893ea16 ] LUCENE-7906 : Add missing override, fixing javadoc
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit dd00446bb912cbb7143d4e8d2394ac9960735f49 in lucene-solr's branch refs/heads/master from Karl Wright
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=dd00446 ]

        LUCENE-7906: Add missing constructor to make precommit happy

        Show
        jira-bot ASF subversion and git services added a comment - Commit dd00446bb912cbb7143d4e8d2394ac9960735f49 in lucene-solr's branch refs/heads/master from Karl Wright [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=dd00446 ] LUCENE-7906 : Add missing constructor to make precommit happy
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit daa12d18cade4fec523d53ceac3a3594aee017f4 in lucene-solr's branch refs/heads/branch_6x from Karl Wright
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=daa12d1 ]

        LUCENE-7906: Add missing constructor to make precommit happy

        Show
        jira-bot ASF subversion and git services added a comment - Commit daa12d18cade4fec523d53ceac3a3594aee017f4 in lucene-solr's branch refs/heads/branch_6x from Karl Wright [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=daa12d1 ] LUCENE-7906 : Add missing constructor to make precommit happy
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 752d57ef4e2e128fd03dccb681a3e0a05557f3b5 in lucene-solr's branch refs/heads/branch_7x from Karl Wright
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=752d57e ]

        LUCENE-7906: Add missing constructor to make precommit happy

        Show
        jira-bot ASF subversion and git services added a comment - Commit 752d57ef4e2e128fd03dccb681a3e0a05557f3b5 in lucene-solr's branch refs/heads/branch_7x from Karl Wright [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=752d57e ] LUCENE-7906 : Add missing constructor to make precommit happy
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit ac1727d9480645f28c8ea749b622b3f60c8951d7 in lucene-solr's branch refs/heads/branch_7x from Karl Wright
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=ac1727d ]

        LUCENE-7906: More fixes for precommit breakage: can't use Math.toRadians

        Show
        jira-bot ASF subversion and git services added a comment - Commit ac1727d9480645f28c8ea749b622b3f60c8951d7 in lucene-solr's branch refs/heads/branch_7x from Karl Wright [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=ac1727d ] LUCENE-7906 : More fixes for precommit breakage: can't use Math.toRadians
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 23d383765b1effaf1df8fd5a0132d32dd17b6840 in lucene-solr's branch refs/heads/master from Karl Wright
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=23d3837 ]

        LUCENE-7906: More fixes for precommit breakage: can't use Math.toRadians

        Show
        jira-bot ASF subversion and git services added a comment - Commit 23d383765b1effaf1df8fd5a0132d32dd17b6840 in lucene-solr's branch refs/heads/master from Karl Wright [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=23d3837 ] LUCENE-7906 : More fixes for precommit breakage: can't use Math.toRadians
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 08be919d3e4029f7ab53dc37a62e7cfd764b03e7 in lucene-solr's branch refs/heads/branch_6x from Karl Wright
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=08be919d ]

        LUCENE-7906: More fixes for precommit breakage: can't use Math.toRadians

        Show
        jira-bot ASF subversion and git services added a comment - Commit 08be919d3e4029f7ab53dc37a62e7cfd764b03e7 in lucene-solr's branch refs/heads/branch_6x from Karl Wright [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=08be919d ] LUCENE-7906 : More fixes for precommit breakage: can't use Math.toRadians
        Hide
        ivera Ignacio Vera added a comment -

        thanks for fixing these issues. I found the ticket about the issue of using Math.toRadians and I understand why it is troblesome. Hopefully it will be stable now!

        Show
        ivera Ignacio Vera added a comment - thanks for fixing these issues. I found the ticket about the issue of using Math.toRadians and I understand why it is troblesome. Hopefully it will be stable now!
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 18e1b40b1c4a15379da865fef05f9a58a2dbdcbf in lucene-solr's branch refs/heads/master from Karl Wright
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=18e1b40 ]

        LUCENE-7918: Revamp the API for composites so that it's generic and useful for many kinds of shapes. Committed (as was LUCENE-7906) on behalf of Ignacio Vera.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 18e1b40b1c4a15379da865fef05f9a58a2dbdcbf in lucene-solr's branch refs/heads/master from Karl Wright [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=18e1b40 ] LUCENE-7918 : Revamp the API for composites so that it's generic and useful for many kinds of shapes. Committed (as was LUCENE-7906 ) on behalf of Ignacio Vera.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit e3f5aed562b2e398a40ea85ee8264d28fc0e023b in lucene-solr's branch refs/heads/branch_6x from Karl Wright
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=e3f5aed ]

        LUCENE-7918: Revamp the API for composites so that it's generic and useful for many kinds of shapes. Committed (as was LUCENE-7906) on behalf of Ignacio Vera.

        Show
        jira-bot ASF subversion and git services added a comment - Commit e3f5aed562b2e398a40ea85ee8264d28fc0e023b in lucene-solr's branch refs/heads/branch_6x from Karl Wright [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=e3f5aed ] LUCENE-7918 : Revamp the API for composites so that it's generic and useful for many kinds of shapes. Committed (as was LUCENE-7906 ) on behalf of Ignacio Vera.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 7d1c7e757668337ec33bc543c9718320fd6974fe in lucene-solr's branch refs/heads/branch_7x from Karl Wright
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7d1c7e7 ]

        LUCENE-7918: Revamp the API for composites so that it's generic and useful for many kinds of shapes. Committed (as was LUCENE-7906) on behalf of Ignacio Vera.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 7d1c7e757668337ec33bc543c9718320fd6974fe in lucene-solr's branch refs/heads/branch_7x from Karl Wright [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=7d1c7e7 ] LUCENE-7918 : Revamp the API for composites so that it's generic and useful for many kinds of shapes. Committed (as was LUCENE-7906 ) on behalf of Ignacio Vera.
        Hide
        ivera Ignacio Vera added a comment -

        I think I found an issue with degenerate geopoints. If the degenerate Geopoint lays on the boundary of a shape, the relationships between the objects are not symetrical:

        The bounding box thinks it contains the degenerated point.
        The degenerated point thinks it intersects the shape.

        Not sure what is the right answer:

        If the answer is WITHIN/CONTAINS: Method intersects(Plane plane, ....) should always return false.

        If the answer is OVERLAPS/OVERLAPS: Method intersects(GeoShape geoShape) should pass a plane for a degenerated GeoPoint. In this case I think we should consider creating a Geoshape containing a point and the plane.

        Thanks,

        I.

        Show
        ivera Ignacio Vera added a comment - I think I found an issue with degenerate geopoints. If the degenerate Geopoint lays on the boundary of a shape, the relationships between the objects are not symetrical: The bounding box thinks it contains the degenerated point. The degenerated point thinks it intersects the shape. Not sure what is the right answer: If the answer is WITHIN/CONTAINS: Method intersects(Plane plane, ....) should always return false. If the answer is OVERLAPS/OVERLAPS: Method intersects(GeoShape geoShape) should pass a plane for a degenerated GeoPoint. In this case I think we should consider creating a Geoshape containing a point and the plane. Thanks, I.
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        Ignacio Vera, a point that is on the edge of a shape should be considered to be within the shape. So the proper answer is WITHIN/CONTAINS.

        Can you create a new ticket for this issue? Thanks!

        Show
        kwright@metacarta.com Karl Wright added a comment - Ignacio Vera , a point that is on the edge of a shape should be considered to be within the shape. So the proper answer is WITHIN/CONTAINS. Can you create a new ticket for this issue? Thanks!
        Hide
        ivera Ignacio Vera added a comment -

        Hi Karl Wright,

        I attach a new version of the random shape generator class. I think it belongs to this ticket. Main changes are:

        • Rename the class to "Geo3d"
        • Added two new shapes, GeoPointShape and degenerate path.
        • Improve test by providing feedback in case they fail

        I "beasted" the test and run precommit successfully.

        Thanks!

        Show
        ivera Ignacio Vera added a comment - Hi Karl Wright , I attach a new version of the random shape generator class. I think it belongs to this ticket. Main changes are: Rename the class to "Geo3d" Added two new shapes, GeoPointShape and degenerate path. Improve test by providing feedback in case they fail I "beasted" the test and run precommit successfully. Thanks!
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        Hi Ignacio Vera, I can't get this patch to apply; it's created from the wrong point within the hierarchy. Can you recreated it and reattach? Thanks!

        Show
        kwright@metacarta.com Karl Wright added a comment - Hi Ignacio Vera , I can't get this patch to apply; it's created from the wrong point within the hierarchy. Can you recreated it and reattach? Thanks!
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        The problem seems to be that there's an earlier patch that's missing:

        error: lucene/spatial3d/src/test/org/apache/lucene/spatial3d/geom/RandomGeo3dShapeGenerator.java: No such file or directory
        
        Show
        kwright@metacarta.com Karl Wright added a comment - The problem seems to be that there's an earlier patch that's missing: error: lucene/spatial3d/src/test/org/apache/lucene/spatial3d/geom/RandomGeo3dShapeGenerator.java: No such file or directory
        Hide
        ivera Ignacio Vera added a comment -

        sorry about this Karl Wright,

        I have the feeling I generated the patch and the project was not clean. Attached a new version, I hope this work.

        Thanks!

        Show
        ivera Ignacio Vera added a comment - sorry about this Karl Wright , I have the feeling I generated the patch and the project was not clean. Attached a new version, I hope this work. Thanks!
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        Ignacio Vera, I'm getting the same error.
        Maybe you can do a fresh checkout and copy your changes into it before generating a new patch?

        Show
        kwright@metacarta.com Karl Wright added a comment - Ignacio Vera , I'm getting the same error. Maybe you can do a fresh checkout and copy your changes into it before generating a new patch?
        Hide
        ivera Ignacio Vera added a comment -

        I have recreated the patch from scratch and it is generated using plain git command. This should work. sorry again!

        Show
        ivera Ignacio Vera added a comment - I have recreated the patch from scratch and it is generated using plain git command. This should work. sorry again!
        Hide
        dsmiley David Smiley added a comment -

        As an aside, this is a good illustration on the weaknesses of patch files compared to GitHub Pull Requests.

        Show
        dsmiley David Smiley added a comment - As an aside, this is a good illustration on the weaknesses of patch files compared to GitHub Pull Requests.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit cd425d609cee8bcea6dbfeab8b3d42b1ce48eb40 in lucene-solr's branch refs/heads/master from Karl Wright
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=cd425d6 ]

        LUCENE-7906: Add new shapes to testing paradigm. Committed on behalf of Ignacio Vera.

        Show
        jira-bot ASF subversion and git services added a comment - Commit cd425d609cee8bcea6dbfeab8b3d42b1ce48eb40 in lucene-solr's branch refs/heads/master from Karl Wright [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=cd425d6 ] LUCENE-7906 : Add new shapes to testing paradigm. Committed on behalf of Ignacio Vera.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 38474e047dd3a5ac9dbe5fbb96595a0a74fec8da in lucene-solr's branch refs/heads/branch_6x from Karl Wright
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=38474e0 ]

        LUCENE-7906: Add new shapes to testing paradigm. Committed on behalf of Ignacio Vera.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 38474e047dd3a5ac9dbe5fbb96595a0a74fec8da in lucene-solr's branch refs/heads/branch_6x from Karl Wright [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=38474e0 ] LUCENE-7906 : Add new shapes to testing paradigm. Committed on behalf of Ignacio Vera.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 3c7ceab4183aace9f31363a220f1f4dfc952ccbe in lucene-solr's branch refs/heads/branch_7x from Karl Wright
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=3c7ceab ]

        LUCENE-7906: Add new shapes to testing paradigm. Committed on behalf of Ignacio Vera.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 3c7ceab4183aace9f31363a220f1f4dfc952ccbe in lucene-solr's branch refs/heads/branch_7x from Karl Wright [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=3c7ceab ] LUCENE-7906 : Add new shapes to testing paradigm. Committed on behalf of Ignacio Vera.
        Hide
        ivera Ignacio Vera added a comment -

        I think something did not go as expected:

        lucene/spatial3d/src/test/org/apache/lucene/spatial3d/geom/RandomBinaryCodecTest.java diff | blob | history
        lucene/spatial3d/src/test/org/apache/lucene/spatial3d/geom/RandomGeoShapeGenerator.java [deleted file] blob | history
        lucene/spatial3d/src/test/org/apache/lucene/spatial3d/geom/RandomGeoShapeRelationshipTest.java diff | blob | history

        The RandomGeoShapeGenerator.java class was deleted but not replaced with RandomGeo3dShapeGenerator.java

        Show
        ivera Ignacio Vera added a comment - I think something did not go as expected: lucene/spatial3d/src/test/org/apache/lucene/spatial3d/geom/RandomBinaryCodecTest.java diff | blob | history lucene/spatial3d/src/test/org/apache/lucene/spatial3d/geom/RandomGeoShapeGenerator.java [deleted file] blob | history lucene/spatial3d/src/test/org/apache/lucene/spatial3d/geom/RandomGeoShapeRelationshipTest.java diff | blob | history The RandomGeoShapeGenerator.java class was deleted but not replaced with RandomGeo3dShapeGenerator.java
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit e44ee16c74832031556e0c2c2e0b9fd0588e968b in lucene-solr's branch refs/heads/master from Karl Wright
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=e44ee16 ]

        LUCENE-7906: Include missing file

        Show
        jira-bot ASF subversion and git services added a comment - Commit e44ee16c74832031556e0c2c2e0b9fd0588e968b in lucene-solr's branch refs/heads/master from Karl Wright [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=e44ee16 ] LUCENE-7906 : Include missing file
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 1a55be800bd63efb11098c7e8929d8ff88232ec7 in lucene-solr's branch refs/heads/branch_6x from Karl Wright
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=1a55be8 ]

        LUCENE-7906: Include missing file

        Show
        jira-bot ASF subversion and git services added a comment - Commit 1a55be800bd63efb11098c7e8929d8ff88232ec7 in lucene-solr's branch refs/heads/branch_6x from Karl Wright [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=1a55be8 ] LUCENE-7906 : Include missing file
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 665f84843c8ac75d997ee107255bc51426226e5c in lucene-solr's branch refs/heads/branch_7x from Karl Wright
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=665f848 ]

        LUCENE-7906: Include missing file

        Show
        jira-bot ASF subversion and git services added a comment - Commit 665f84843c8ac75d997ee107255bc51426226e5c in lucene-solr's branch refs/heads/branch_7x from Karl Wright [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=665f848 ] LUCENE-7906 : Include missing file
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        Ignacio Vera, for some reason git commit -a did not pick up the added file. My apologies. I had to explicitly add it. Should be fixed now.

        Show
        kwright@metacarta.com Karl Wright added a comment - Ignacio Vera , for some reason git commit -a did not pick up the added file. My apologies. I had to explicitly add it. Should be fixed now.
        Hide
        ivera Ignacio Vera added a comment -

        Hi Karl Wright,

        I have realized there is something no correct in the random shape generator. Currently it is only building shapes in a portion of the planet! (lat and lon between 0 and 1) Attached is the fix to build shapes in the whole planet.

        I "beasted" the test and there were no surpises and performance seems to improve.

        Show
        ivera Ignacio Vera added a comment - Hi Karl Wright , I have realized there is something no correct in the random shape generator. Currently it is only building shapes in a portion of the planet! (lat and lon between 0 and 1) Attached is the fix to build shapes in the whole planet. I "beasted" the test and there were no surpises and performance seems to improve.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 12b0acdf7c03a905151300c9b635c23e9453e26d in lucene-solr's branch refs/heads/master from Karl Wright
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=12b0acd ]

        LUCENE-7906: Fix random shape generator. Committed on behalf of Ignacio Vera.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 12b0acdf7c03a905151300c9b635c23e9453e26d in lucene-solr's branch refs/heads/master from Karl Wright [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=12b0acd ] LUCENE-7906 : Fix random shape generator. Committed on behalf of Ignacio Vera.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 1ed0d4e5fb24ee82d3c0931051ead663d468ec62 in lucene-solr's branch refs/heads/branch_6x from Karl Wright
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=1ed0d4e ]

        LUCENE-7906: Fix random shape generator. Committed on behalf of Ignacio Vera.

        Show
        jira-bot ASF subversion and git services added a comment - Commit 1ed0d4e5fb24ee82d3c0931051ead663d468ec62 in lucene-solr's branch refs/heads/branch_6x from Karl Wright [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=1ed0d4e ] LUCENE-7906 : Fix random shape generator. Committed on behalf of Ignacio Vera.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit d3982f0dab77bcf31a29e3425cb471fcc1019671 in lucene-solr's branch refs/heads/branch_7x from Karl Wright
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d3982f0 ]

        LUCENE-7906: Fix random shape generator. Committed on behalf of Ignacio Vera.

        Show
        jira-bot ASF subversion and git services added a comment - Commit d3982f0dab77bcf31a29e3425cb471fcc1019671 in lucene-solr's branch refs/heads/branch_7x from Karl Wright [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d3982f0 ] LUCENE-7906 : Fix random shape generator. Committed on behalf of Ignacio Vera.
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        Ignacio Vera, I've committed this latest patch.

        This ticket has been ongoing now for six weeks. I think it's time to open new tickets when you find new problems. So let's do that in the future.

        Show
        kwright@metacarta.com Karl Wright added a comment - Ignacio Vera , I've committed this latest patch. This ticket has been ongoing now for six weeks. I think it's time to open new tickets when you find new problems. So let's do that in the future.
        Hide
        ivera Ignacio Vera added a comment -

        Agree. Thanks!

        Show
        ivera Ignacio Vera added a comment - Agree. Thanks!

          People

          • Assignee:
            kwright@metacarta.com Karl Wright
            Reporter:
            ivera Ignacio Vera
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:

              Development