Lucene - Core
  1. Lucene - Core
  2. LUCENE-6578

Geo3d: arcDistanceToShape() method may be useful

    Details

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

      Description

      I've got an application that seems like it may need the ability to compute a new kind of arc distance, from a GeoPoint to the nearest edge/point of a GeoShape. Adding this method to the interface, and corresponding implementations, would increase the utility of the package for ranking purposes.

      1. LUCENE-6578.patch
        112 kB
        Karl Wright
      2. LUCENE-6578.patch
        111 kB
        Karl Wright
      3. LUCENE-6578.patch
        110 kB
        Karl Wright
      4. LUCENE-6578-dws.patch
        144 kB
        David Smiley

        Activity

        Hide
        Karl Wright added a comment -

        Preliminary patch with the code changes desired, but no tests yet.

        Tests will be included in an updated patch.

        Show
        Karl Wright added a comment - Preliminary patch with the code changes desired, but no tests yet. Tests will be included in an updated patch.
        Hide
        Karl Wright added a comment -

        Patch against updated trunk, with tests added.

        Show
        Karl Wright added a comment - Patch against updated trunk, with tests added.
        Hide
        Karl Wright added a comment -

        The patch also fixes a place where we did not take the planet model into account, but should have, during the computation of linear distances.

        Show
        Karl Wright added a comment - The patch also fixes a place where we did not take the planet model into account, but should have, during the computation of linear distances.
        Hide
        Karl Wright added a comment -

        David Smiley: I am done with this patch. It should apply against the trunk version of geo3d.

        FWIW, this feature is not one that I believe spatial4j currently supports, but it's one that we find very useful for post-search ranking.

        Show
        Karl Wright added a comment - David Smiley : I am done with this patch. It should apply against the trunk version of geo3d. FWIW, this feature is not one that I believe spatial4j currently supports, but it's one that we find very useful for post-search ranking.
        Hide
        David Smiley added a comment -

        Sounds cool, Karl. It's kinda crunch time for me right now to get some work done but I'll get to this eventually but not soon.

        Show
        David Smiley added a comment - Sounds cool, Karl. It's kinda crunch time for me right now to get some work done but I'll get to this eventually but not soon.
        Hide
        David Smiley added a comment -

        I took a brief look. Wow this patch is big. But it contains a lot of repetition, that will make maintenance harder. One type of repetition is the Javadocs. If a superclass/interface defines a method, then put javadocs only there and don't copy-paste to each implementing class (unless there's truly something unique to say). The other type of repetition is the distance algorithm between several added methods of most instances of any particular shape instance. For example I see the same method with method calls changed from "normalDistance" to "normalDistanceSquared" but otherwise appears identical. Can you please think of a way to approach this with less repetition and thus more maintainability? Also, a few of these added methods might have suitable default implementations in a superclass, like computeOutsideSquaredNormalDistance(Geopoint pt) to simply call by x,y,z.

        Show
        David Smiley added a comment - I took a brief look. Wow this patch is big. But it contains a lot of repetition, that will make maintenance harder. One type of repetition is the Javadocs. If a superclass/interface defines a method, then put javadocs only there and don't copy-paste to each implementing class (unless there's truly something unique to say). The other type of repetition is the distance algorithm between several added methods of most instances of any particular shape instance. For example I see the same method with method calls changed from "normalDistance" to "normalDistanceSquared" but otherwise appears identical. Can you please think of a way to approach this with less repetition and thus more maintainability? Also, a few of these added methods might have suitable default implementations in a superclass, like computeOutsideSquaredNormalDistance(Geopoint pt) to simply call by x,y,z.
        Hide
        Karl Wright added a comment -

        For example I see the same method with method calls changed from "normalDistance" to "normalDistanceSquared" but otherwise appears identical. Can you please think of a way to approach this with less repetition and thus more maintainability?

        The whole purpose of having multiple distance metrics is performance. These computations are expected to be used at document scoring time. Saving even a single expensive sqrt is important in that context, which is why the implementations look as they do.

        Object creation for the purposes of reducing code "duplication according to pattern" is also a bad idea for the same reason. Object creation would be required for any kind of more complete code-sharing approach.

        Mapping the xxx(GeoPoint pt) to xxx(x,y,z), though, is something doable and I will look into it. I will also remove the javadoc, although I find it odd that that would be considered important for Lucene.

        Show
        Karl Wright added a comment - For example I see the same method with method calls changed from "normalDistance" to "normalDistanceSquared" but otherwise appears identical. Can you please think of a way to approach this with less repetition and thus more maintainability? The whole purpose of having multiple distance metrics is performance. These computations are expected to be used at document scoring time. Saving even a single expensive sqrt is important in that context, which is why the implementations look as they do. Object creation for the purposes of reducing code "duplication according to pattern" is also a bad idea for the same reason. Object creation would be required for any kind of more complete code-sharing approach. Mapping the xxx(GeoPoint pt) to xxx(x,y,z), though, is something doable and I will look into it. I will also remove the javadoc, although I find it odd that that would be considered important for Lucene.
        Hide
        Karl Wright added a comment -

        Michael McCandless: I'd also like your opinion as to whether I should remove javadoc from methods that implement an interface; this seems questionable to me. Does lucene have a best practice for this kind of thing?

        Show
        Karl Wright added a comment - Michael McCandless : I'd also like your opinion as to whether I should remove javadoc from methods that implement an interface; this seems questionable to me. Does lucene have a best practice for this kind of thing?
        Hide
        Karl Wright added a comment -

        Revised patch cutting back on the size of the diff.

        Show
        Karl Wright added a comment - Revised patch cutting back on the size of the diff.
        Hide
        Michael McCandless added a comment -

        Karl Wright we generally don't repeat the javadocs if it's just the same thing that the interface had in the javadocs for that method ...

        Show
        Michael McCandless added a comment - Karl Wright we generally don't repeat the javadocs if it's just the same thing that the interface had in the javadocs for that method ...
        Hide
        Michael McCandless added a comment -

        But I don't think that should block committing this! Such improvements can be made later ...

        Show
        Michael McCandless added a comment - But I don't think that should block committing this! Such improvements can be made later ...
        Hide
        Karl Wright added a comment -

        Thanks, I will keep that in mind for the future.

        Show
        Karl Wright added a comment - Thanks, I will keep that in mind for the future.
        Hide
        Karl Wright added a comment -

        I stripped the comments out and introduced a full abstract base class hierarchy so we could do at least minimal code sharing. There are downsides but they aren't severe. Let's see what David says.

        Show
        Karl Wright added a comment - I stripped the comments out and introduced a full abstract base class hierarchy so we could do at least minimal code sharing. There are downsides but they aren't severe. Let's see what David says.
        Hide
        David Smiley added a comment -

        I'm not in a hurry to commit anything that has this amount of code redundancy – sorry. Now is the time to address it; not in an inevitable future issue. Apparently Mike feels differently. It's great seeing reduced redundant javadocs in the latest patch – thanks. By the way, new patches should generally be named the same in JIRA.

        Karl, a specific idea that would reduce the code here a ton is a simple interface like:

        interface DistCalculator {
          double calcDistance(x, y, z);
        }
        

        Then each shape needs... perhaps just a single method taking this? There would be no extra object allocation per distance run.

        Show
        David Smiley added a comment - I'm not in a hurry to commit anything that has this amount of code redundancy – sorry. Now is the time to address it; not in an inevitable future issue. Apparently Mike feels differently. It's great seeing reduced redundant javadocs in the latest patch – thanks. By the way, new patches should generally be named the same in JIRA. Karl, a specific idea that would reduce the code here a ton is a simple interface like: interface DistCalculator { double calcDistance(x, y, z); } Then each shape needs... perhaps just a single method taking this? There would be no extra object allocation per distance run.
        Hide
        Karl Wright added a comment -

        So, I don't understand how introduction of a single interface can somehow create six distance methods in every object? Without any additional object creation? Can you please be more specific?

        Show
        Karl Wright added a comment - So, I don't understand how introduction of a single interface can somehow create six distance methods in every object? Without any additional object creation? Can you please be more specific?
        Hide
        Karl Wright added a comment -

        Also, FWIW, it's rather timeconsuming to make multiple iterative little tweaky changes to every single shape class. It's much much better to arrive at a solution that is acceptable before I go off and code anything else. So please, let us do that rather than waste my time any further.

        Show
        Karl Wright added a comment - Also, FWIW, it's rather timeconsuming to make multiple iterative little tweaky changes to every single shape class. It's much much better to arrive at a solution that is acceptable before I go off and code anything else. So please, let us do that rather than waste my time any further.
        Hide
        Karl Wright added a comment -

        Ok, the nearest thing I can think of to what you may be suggesting is as follows:

        • a distance calculator interface, with methods for computing plane distance and point distance
        • six implementations of that interface, one for linear, normal, linear squared, normal squared, and arc
        • GeoOutsideDistance then has two methods: one for GeoPoint + distcalcinterface and one for (x,y,z) + distcalcinterface

        David Smiley Is that what you were talking about?

        Show
        Karl Wright added a comment - Ok, the nearest thing I can think of to what you may be suggesting is as follows: a distance calculator interface, with methods for computing plane distance and point distance six implementations of that interface, one for linear, normal, linear squared, normal squared, and arc GeoOutsideDistance then has two methods: one for GeoPoint + distcalcinterface and one for (x,y,z) + distcalcinterface David Smiley Is that what you were talking about?
        Hide
        Karl Wright added a comment -

        The distance calculator interface would look then like this:

        public interface DistanceStyle {
          
          /** Compute the distance from a point to another point.
           * @param point1 Starting point
           * @param point2 Final point
           * @return the distance
           */
          public double computeDistance(final GeoPoint point1, final GeoPoint point2);
          
          /** Compute the distance from a point to another point.
           * @param point1 Starting point
           * @param x2 Final point x
           * @param y2 Final point y
           * @param z2 Final point z
           * @return the distance
           */
          public double computeDistance(final GeoPoint point1, final double x2, final double y2, final double z2);
        
          /** Compute the distance from a plane to a point.
           * @param planetModel The planet model
           * @param plane The plane
           * @param point The point
           * @return the distance
           */
          public double computeDistance(final PlanetModel planetModel, final Plane plane, final GeoPoint point);
          
          /** Compute the distance from a plane to a point.
           * @param planetModel The planet model
           * @param plane The plane
           * @param x The point x
           * @param y The point y
           * @param z The point z
           * @return the distance
           */
          public double computeDistance(final PlanetModel planetModel, final Plane plane, final double x, final double y, final double z);
        
        }
        
        Show
        Karl Wright added a comment - The distance calculator interface would look then like this: public interface DistanceStyle { /** Compute the distance from a point to another point. * @param point1 Starting point * @param point2 Final point * @ return the distance */ public double computeDistance( final GeoPoint point1, final GeoPoint point2); /** Compute the distance from a point to another point. * @param point1 Starting point * @param x2 Final point x * @param y2 Final point y * @param z2 Final point z * @ return the distance */ public double computeDistance( final GeoPoint point1, final double x2, final double y2, final double z2); /** Compute the distance from a plane to a point. * @param planetModel The planet model * @param plane The plane * @param point The point * @ return the distance */ public double computeDistance( final PlanetModel planetModel, final Plane plane, final GeoPoint point); /** Compute the distance from a plane to a point. * @param planetModel The planet model * @param plane The plane * @param x The point x * @param y The point y * @param z The point z * @ return the distance */ public double computeDistance( final PlanetModel planetModel, final Plane plane, final double x, final double y, final double z); }
        Hide
        David Smiley added a comment -

        Also, FWIW, it's rather timeconsuming to make multiple iterative little tweaky changes to every single shape class. It's much much better to arrive at a solution that is acceptable before I go off and code anything else. So please, let us do that rather than waste my time any further.

        Yes definitely. I hope I'm not wasting your time. I like that you've posted some code here to discuss rather than fully integrating it which would take more time and may need to be done again if there's feedback.

        Ok, the nearest thing I can think of to what you may be suggesting is as follows:

        Yes, this is basically what I had in mind. I'm curious why the GeoPoint & x/y/z methods are duplicated; is it because some distance algorithms could leverage lat/lon being pre-calculated on GeoPoint? If so, that makes sense. Might you consider another interface name that doesn't have "Style"... DistanceType perhaps? When I think "Style" I think formatting. But it's not important (obviously).

        Show
        David Smiley added a comment - Also, FWIW, it's rather timeconsuming to make multiple iterative little tweaky changes to every single shape class. It's much much better to arrive at a solution that is acceptable before I go off and code anything else. So please, let us do that rather than waste my time any further. Yes definitely. I hope I'm not wasting your time. I like that you've posted some code here to discuss rather than fully integrating it which would take more time and may need to be done again if there's feedback. Ok, the nearest thing I can think of to what you may be suggesting is as follows: Yes, this is basically what I had in mind. I'm curious why the GeoPoint & x/y/z methods are duplicated; is it because some distance algorithms could leverage lat/lon being pre-calculated on GeoPoint? If so, that makes sense. Might you consider another interface name that doesn't have "Style"... DistanceType perhaps? When I think "Style" I think formatting. But it's not important (obviously).
        Hide
        Michael McCandless added a comment -

        I'm not in a hurry to commit anything that has this amount of code redundancy – sorry. Now is the time to address it; not in an inevitable future issue. Apparently Mike feels differently.

        Please don't over-read into my comments. I haven't looked at this patch at all yet.

        I was simply responding directly to Karl's question "should @Override of an interface method have its own javadocs": generally they should not, unless the impl has something different to document, but that this should not block committing an otherwise good change.

        Show
        Michael McCandless added a comment - I'm not in a hurry to commit anything that has this amount of code redundancy – sorry. Now is the time to address it; not in an inevitable future issue. Apparently Mike feels differently. Please don't over-read into my comments. I haven't looked at this patch at all yet. I was simply responding directly to Karl's question "should @Override of an interface method have its own javadocs": generally they should not, unless the impl has something different to document, but that this should not block committing an otherwise good change.
        Hide
        Michael McCandless added a comment -

        I've only glanced at the patch so far, but abstractions are hard to get right until a given API has seen a lot of usage, and I feel like spatial module already has too many levels of abstraction now, so I think the disagreement here is good evidence that geo3d should live in sandbox for starters, where it's free to be more "sandy", it can more freely iterate / change its abstractions, etc.

        Show
        Michael McCandless added a comment - I've only glanced at the patch so far, but abstractions are hard to get right until a given API has seen a lot of usage, and I feel like spatial module already has too many levels of abstraction now, so I think the disagreement here is good evidence that geo3d should live in sandbox for starters, where it's free to be more "sandy", it can more freely iterate / change its abstractions, etc.
        Hide
        Karl Wright added a comment -

        I'm curious why the GeoPoint & x/y/z methods are duplicated

        It's because I want to avoid creating a GeoPoint object simply to compute a distance. We could have just the x,y,z versions of the methods, but having the GeoPoint variant is convenient to have as well.

        Show
        Karl Wright added a comment - I'm curious why the GeoPoint & x/y/z methods are duplicated It's because I want to avoid creating a GeoPoint object simply to compute a distance. We could have just the x,y,z versions of the methods, but having the GeoPoint variant is convenient to have as well.
        Hide
        Michael McCandless added a comment -

        I've looked at the patch a bit more and it already adds an interface (GeoOutsideDistance) for these new methods?

        Why should we make the abstractions even deeper (adding DistanceStyle/DistanceType)? I think all the abstractions the spatial module already has (even in its test cases) make it very hard to approach for newcomers ... I don't think we should pollute geo3d in the same way? Abstractions are costly, and we should err towards "too few abstractions" when unsure.

        I'll open an issue to move geo3d to sandbox ... I think this is a better home for it for now, letting users know it's very new and APIs are subject to change, and letting Karl Wright make faster iterations to it. It would also un-block issues like LUCENE-6480 ... I think a postings and BKD tree integration based on geo3d would be very powerful.

        Show
        Michael McCandless added a comment - I've looked at the patch a bit more and it already adds an interface (GeoOutsideDistance) for these new methods? Why should we make the abstractions even deeper (adding DistanceStyle/DistanceType)? I think all the abstractions the spatial module already has (even in its test cases) make it very hard to approach for newcomers ... I don't think we should pollute geo3d in the same way? Abstractions are costly, and we should err towards "too few abstractions" when unsure. I'll open an issue to move geo3d to sandbox ... I think this is a better home for it for now, letting users know it's very new and APIs are subject to change, and letting Karl Wright make faster iterations to it. It would also un-block issues like LUCENE-6480 ... I think a postings and BKD tree integration based on geo3d would be very powerful.
        Hide
        Karl Wright added a comment -

        I'm open to this, as soon as this patch is done and committed.
        I hope to have something late today or early tomorrow that should address David's current concerns, except perhaps his concern about choice of interface name.

        Show
        Karl Wright added a comment - I'm open to this, as soon as this patch is done and committed. I hope to have something late today or early tomorrow that should address David's current concerns, except perhaps his concern about choice of interface name.
        Hide
        Michael McCandless added a comment -

        I'll open an issue to move geo3d to sandbox

        I opened LUCENE-6607.

        Show
        Michael McCandless added a comment - I'll open an issue to move geo3d to sandbox I opened LUCENE-6607 .
        Hide
        Karl Wright added a comment -

        Attaching revision (yes, I know I should just replace the patch, but people seem to want to compare patches as well as examine the patch itself)

        Show
        Karl Wright added a comment - Attaching revision (yes, I know I should just replace the patch, but people seem to want to compare patches as well as examine the patch itself)
        Hide
        David Smiley added a comment -

        Aha; makes sense. I guess I'm just wondering if the GeoPoint convenience method need only be on the shape then?

        Show
        David Smiley added a comment - Aha; makes sense. I guess I'm just wondering if the GeoPoint convenience method need only be on the shape then?
        Hide
        David Smiley added a comment -

        Please don't over-read into my comments. I haven't looked at this patch at all yet.

        Okay sorry.

        Show
        David Smiley added a comment - Please don't over-read into my comments. I haven't looked at this patch at all yet. Okay sorry.
        Hide
        David Smiley added a comment -

        I sympathize with Michael McCandless's point on GeoOutsideDistance; perhaps instead the methods could go on GeoShape, and then a shape that doesn't support them could throw UnsupportedOperationException. I have no strong opinion either way. Any way, it's "Progress not perfection", ehh Mike?

        One thing abstractions can do, is reduce code duplication. The interface "DistanceStyle" does exactly that – there are many ways to compute a distance spatially. This change reduced the patch file by ~40%. Does anyone argue this specific change was anything less than an improvement?

        Attaching revision (yes, I know I should just replace the patch, but people seem to want to compare patches as well as examine the patch itself)

        I'll look at your patch tonight. FYI JIRA does not overwrite patches by the same name; each one is available. See LUCENE-6191 for an example.

        Show
        David Smiley added a comment - I sympathize with Michael McCandless 's point on GeoOutsideDistance; perhaps instead the methods could go on GeoShape, and then a shape that doesn't support them could throw UnsupportedOperationException. I have no strong opinion either way. Any way, it's "Progress not perfection", ehh Mike? One thing abstractions can do, is reduce code duplication. The interface "DistanceStyle" does exactly that – there are many ways to compute a distance spatially. This change reduced the patch file by ~40%. Does anyone argue this specific change was anything less than an improvement? Attaching revision (yes, I know I should just replace the patch, but people seem to want to compare patches as well as examine the patch itself) I'll look at your patch tonight. FYI JIRA does not overwrite patches by the same name; each one is available. See LUCENE-6191 for an example.
        Hide
        Karl Wright added a comment -

        This patch revamps GeoDistance as well as GeoOutsideDistance methods.

        Show
        Karl Wright added a comment - This patch revamps GeoDistance as well as GeoOutsideDistance methods.
        Hide
        Karl Wright added a comment -

        Less expensive GeoPath distance computation.

        Show
        Karl Wright added a comment - Less expensive GeoPath distance computation.
        Hide
        Michael McCandless added a comment -

        One thing abstractions can do, is reduce code duplication. The interface "DistanceStyle" does exactly that – there are many ways to compute a distance spatially. This change reduced the patch file by ~40%. Does anyone argue this specific change was anything less than an improvement?

        Code duplication is not the only metric that matters, and 40% reduction LOC is a fluffed up metric ... how much smaller is it if we just remove the duplicate javadocs?

        In this particular case, I'm not sure I like the new abstraction, but I really do not understand enough to make an informed decision. Can someone succinctly explain what DistanceStyle abstraction represents? This is a burden every newcomer to the geo3d APIs must grapple with / overcome.

        It seems like Karl Wright and David Smiley are happy with this change so I won't block it but I don't really like it. I think there should be a higher bar for inventing new abstractions in the absence of actual usage of a new API.

        Show
        Michael McCandless added a comment - One thing abstractions can do, is reduce code duplication. The interface "DistanceStyle" does exactly that – there are many ways to compute a distance spatially. This change reduced the patch file by ~40%. Does anyone argue this specific change was anything less than an improvement? Code duplication is not the only metric that matters, and 40% reduction LOC is a fluffed up metric ... how much smaller is it if we just remove the duplicate javadocs? In this particular case, I'm not sure I like the new abstraction, but I really do not understand enough to make an informed decision. Can someone succinctly explain what DistanceStyle abstraction represents? This is a burden every newcomer to the geo3d APIs must grapple with / overcome. It seems like Karl Wright and David Smiley are happy with this change so I won't block it but I don't really like it. I think there should be a higher bar for inventing new abstractions in the absence of actual usage of a new API.
        Hide
        David Smiley added a comment -

        This patch is definitely improved; nice job Karl! I'd like to improve it further and get your opinion – I'll post your patch as the base and then a revised one on ReviewBoard so you can easily see the delta and comment on specifics if you choose to. It's a fair amount of code to examine so I won't get to it until tonight starting 11pm. If you post further updates today before 11pm then I'll use that.

        A couple things I want to mention now:

        (1) the choice of "ArcDistance.INSTANCE" as a means of referring to the a distance. It's fine but I prefer the approach in Lucene's FilteredQuery in which the choices are referred via named constants, like FilteredQuery.RANDOM_ACCESS_FILTER_STRATEGY. This avoids seeing "INSTANCE". Taking that approach here would mean having a DistanceStyle.ARC_DISTANCE or perhaps just "DistanceStyle.ARC". What do you think?

        (2) GeoPath.distance() has an implementation that confused me until I realized what it was doing... and I realized that wasn't what came to mind when I saw the distance method. Perhaps this method should be named distanceAlongPath and further document that it's from the beginning?

        Show
        David Smiley added a comment - This patch is definitely improved; nice job Karl! I'd like to improve it further and get your opinion – I'll post your patch as the base and then a revised one on ReviewBoard so you can easily see the delta and comment on specifics if you choose to. It's a fair amount of code to examine so I won't get to it until tonight starting 11pm. If you post further updates today before 11pm then I'll use that. A couple things I want to mention now: (1) the choice of "ArcDistance.INSTANCE" as a means of referring to the a distance. It's fine but I prefer the approach in Lucene's FilteredQuery in which the choices are referred via named constants, like FilteredQuery.RANDOM_ACCESS_FILTER_STRATEGY. This avoids seeing "INSTANCE". Taking that approach here would mean having a DistanceStyle.ARC_DISTANCE or perhaps just "DistanceStyle.ARC". What do you think? (2) GeoPath.distance() has an implementation that confused me until I realized what it was doing... and I realized that wasn't what came to mind when I saw the distance method. Perhaps this method should be named distanceAlongPath and further document that it's from the beginning?
        Hide
        Karl Wright added a comment -

        the choice of "ArcDistance.INSTANCE" as a means of referring to the a distance. It's fine but I prefer the approach in Lucene's FilteredQuery in which the choices are referred via named constants, like FilteredQuery.RANDOM_ACCESS_FILTER_STRATEGY. This avoids seeing "INSTANCE". Taking that approach here would mean having a DistanceStyle.ARC_DISTANCE or perhaps just "DistanceStyle.ARC". What do you think?

        Why not support both? The current strategy is eminently extensible, which is what I like about it.

        GeoPath.distance() has an implementation that confused me until I realized what it was doing... and I realized that wasn't what came to mind when I saw the distance method. Perhaps this method should be named distanceAlongPath and further document that it's from the beginning?

        I construed this as a generic "internal distance" method, which is how it's used in my employer's application. Removing the genericism would bust that application. So I prefer to leave this the way it is, thanks.

        Show
        Karl Wright added a comment - the choice of "ArcDistance.INSTANCE" as a means of referring to the a distance. It's fine but I prefer the approach in Lucene's FilteredQuery in which the choices are referred via named constants, like FilteredQuery.RANDOM_ACCESS_FILTER_STRATEGY. This avoids seeing "INSTANCE". Taking that approach here would mean having a DistanceStyle.ARC_DISTANCE or perhaps just "DistanceStyle.ARC". What do you think? Why not support both? The current strategy is eminently extensible, which is what I like about it. GeoPath.distance() has an implementation that confused me until I realized what it was doing... and I realized that wasn't what came to mind when I saw the distance method. Perhaps this method should be named distanceAlongPath and further document that it's from the beginning? I construed this as a generic "internal distance" method, which is how it's used in my employer's application. Removing the genericism would bust that application. So I prefer to leave this the way it is, thanks.
        Hide
        Karl Wright added a comment -

        Can someone succinctly explain what DistanceStyle abstraction represents?

        There are various different ways of compute a distance metric. Each DistanceStyle implementation is one such way. We support:

        • ArcDistance (accurate angular distance)
        • NormalDistance (perpendicular to a plane/line through the center of the earth)
        • LinearDistance (straight chord distance)
        • NormalSquaredDistance (the square of the normal distance)
        • LinearSquaredDistance (the square of the linear distance)

        The latter two methods are helpful because they require no square roots, so if you need a fast distance estimate, that's what you'd use. I can also imagine adding new distance types in the future, for example SurfaceDistance (which is the expensive ellipsoidal surface distance metric).

        Hope this helps.

        Show
        Karl Wright added a comment - Can someone succinctly explain what DistanceStyle abstraction represents? There are various different ways of compute a distance metric. Each DistanceStyle implementation is one such way. We support: ArcDistance (accurate angular distance) NormalDistance (perpendicular to a plane/line through the center of the earth) LinearDistance (straight chord distance) NormalSquaredDistance (the square of the normal distance) LinearSquaredDistance (the square of the linear distance) The latter two methods are helpful because they require no square roots, so if you need a fast distance estimate, that's what you'd use. I can also imagine adding new distance types in the future, for example SurfaceDistance (which is the expensive ellipsoidal surface distance metric). Hope this helps.
        Hide
        David Smiley added a comment -

        Why not support both? The current strategy is eminently extensible, which is what I like about it.

        Sure; +1.

        I construed this as a generic "internal distance" method, which is how it's used in my employer's application. Removing the genericism would bust that application. So I prefer to leave this the way it is, thanks.

        Okay; this can be handled with javadocs that explain how this shape implements computeDistance.

        I went to apply your patch but ran into some issues in doing so:

        • The patch shows a class was renamed (GeoBaseShape to BasePlanetObject), yet the patch's reference to the file does so in a way that doesn't seem right. The "Index:" line should show the old name, the "---" line should also show the old name, and the "+++" line should show the new name. In your patch, all 3 have the old name. This definitely confused IntelliJ, and I expect it would confuse ReviewBoard too. I hand-edited the patch to change the first two to the old name and that worked. How are you generating the patch?
        • The patch shows GeoBaseMembershipShape (newly added) as extending GeoBaseShape, yet that can’t be since GeoBaseShape should now be named BasePlanetObject. I hand edited it to be BasePlanetObject; but this didn't give me a warm and fuzzy that this patch file is ready for me to work with. ...
        • GeoCircle.getBounds couldn't figure out how to call super.getBounds because there is no superclass implementing that method, just the interface.

        Can you provide a better patch Karl? Was your code compiling at the time you generated the patch?

        Show
        David Smiley added a comment - Why not support both? The current strategy is eminently extensible, which is what I like about it. Sure; +1. I construed this as a generic "internal distance" method, which is how it's used in my employer's application. Removing the genericism would bust that application. So I prefer to leave this the way it is, thanks. Okay; this can be handled with javadocs that explain how this shape implements computeDistance. I went to apply your patch but ran into some issues in doing so: The patch shows a class was renamed (GeoBaseShape to BasePlanetObject), yet the patch's reference to the file does so in a way that doesn't seem right. The "Index:" line should show the old name, the "---" line should also show the old name, and the "+++" line should show the new name. In your patch, all 3 have the old name. This definitely confused IntelliJ, and I expect it would confuse ReviewBoard too. I hand-edited the patch to change the first two to the old name and that worked. How are you generating the patch? The patch shows GeoBaseMembershipShape (newly added) as extending GeoBaseShape, yet that can’t be since GeoBaseShape should now be named BasePlanetObject. I hand edited it to be BasePlanetObject; but this didn't give me a warm and fuzzy that this patch file is ready for me to work with. ... GeoCircle.getBounds couldn't figure out how to call super.getBounds because there is no superclass implementing that method, just the interface. Can you provide a better patch Karl? Was your code compiling at the time you generated the patch?
        Hide
        Karl Wright added a comment -

        Code compiles fine and was produced by svn diff.

        Not sure how I can fix this.

        Sent from my Windows Phone
        From: David Smiley (JIRA)
        Sent: 6/27/2015 2:05 AM
        To: dev@lucene.apache.org
        Subject: [jira] [Commented] (LUCENE-6578) Geo3d: arcDistanceToShape()
        method may be useful

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

        David Smiley commented on LUCENE-6578:
        --------------------------------------

        Why not support both? The current strategy is eminently

        extensible, which is what I like about it.

        Sure; +1.

        I construed this as a generic "internal distance" method, which is

        how it's used in my employer's application. Removing the genericism
        would bust that application. So I prefer to leave this the way it is,
        thanks.

        Okay; this can be handled with javadocs that explain how this shape
        implements computeDistance.

        I went to apply your patch but ran into some issues in doing so:

        • The patch shows a class was renamed (GeoBaseShape to
          BasePlanetObject), yet the patch's reference to the file does so in a
          way that doesn't seem right. The "Index:" line should show the old
          name, the "---" line should also show the old name, and the "+++" line
          should show the new name. In your patch, all 3 have the old name.
          This definitely confused IntelliJ, and I expect it would confuse
          ReviewBoard too. I hand-edited the patch to change the first two to
          the old name and that worked. How are you generating the patch?
        • The patch shows GeoBaseMembershipShape (newly added) as extending
          GeoBaseShape, yet that can’t be since GeoBaseShape should now be named
          BasePlanetObject. I hand edited it to be BasePlanetObject; but this
          didn't give me a warm and fuzzy that this patch file is ready for me
          to work with. ...
        • GeoCircle.getBounds couldn't figure out how to call super.getBounds
          because there is no superclass implementing that method, just the
          interface.

        Can you provide a better patch Karl? Was your code compiling at the
        time you generated the patch?


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

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

        Show
        Karl Wright added a comment - Code compiles fine and was produced by svn diff. Not sure how I can fix this. Sent from my Windows Phone From: David Smiley (JIRA) Sent: 6/27/2015 2:05 AM To: dev@lucene.apache.org Subject: [jira] [Commented] ( LUCENE-6578 ) Geo3d: arcDistanceToShape() method may be useful [ https://issues.apache.org/jira/browse/LUCENE-6578?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14604002#comment-14604002 ] David Smiley commented on LUCENE-6578 : -------------------------------------- Why not support both? The current strategy is eminently extensible, which is what I like about it. Sure; +1. I construed this as a generic "internal distance" method, which is how it's used in my employer's application. Removing the genericism would bust that application. So I prefer to leave this the way it is, thanks. Okay; this can be handled with javadocs that explain how this shape implements computeDistance. I went to apply your patch but ran into some issues in doing so: The patch shows a class was renamed (GeoBaseShape to BasePlanetObject), yet the patch's reference to the file does so in a way that doesn't seem right. The "Index:" line should show the old name, the "---" line should also show the old name, and the "+++" line should show the new name. In your patch, all 3 have the old name. This definitely confused IntelliJ, and I expect it would confuse ReviewBoard too. I hand-edited the patch to change the first two to the old name and that worked. How are you generating the patch? The patch shows GeoBaseMembershipShape (newly added) as extending GeoBaseShape, yet that can’t be since GeoBaseShape should now be named BasePlanetObject. I hand edited it to be BasePlanetObject; but this didn't give me a warm and fuzzy that this patch file is ready for me to work with. ... GeoCircle.getBounds couldn't figure out how to call super.getBounds because there is no superclass implementing that method, just the interface. Can you provide a better patch Karl? Was your code compiling at the time you generated the patch? – This message was sent by Atlassian JIRA (v6.3.4#6332) --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org For additional commands, e-mail: dev-help@lucene.apache.org
        Hide
        Karl Wright added a comment -

        Perhaps try applying the patch with svn patch??

        Sent from my Windows Phone
        From: David Smiley (JIRA)
        Sent: 6/27/2015 2:05 AM
        To: dev@lucene.apache.org
        Subject: [jira] [Commented] (LUCENE-6578) Geo3d: arcDistanceToShape()
        method may be useful

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

        David Smiley commented on LUCENE-6578:
        --------------------------------------

        Why not support both? The current strategy is eminently

        extensible, which is what I like about it.

        Sure; +1.

        I construed this as a generic "internal distance" method, which is

        how it's used in my employer's application. Removing the genericism
        would bust that application. So I prefer to leave this the way it is,
        thanks.

        Okay; this can be handled with javadocs that explain how this shape
        implements computeDistance.

        I went to apply your patch but ran into some issues in doing so:

        • The patch shows a class was renamed (GeoBaseShape to
          BasePlanetObject), yet the patch's reference to the file does so in a
          way that doesn't seem right. The "Index:" line should show the old
          name, the "---" line should also show the old name, and the "+++" line
          should show the new name. In your patch, all 3 have the old name.
          This definitely confused IntelliJ, and I expect it would confuse
          ReviewBoard too. I hand-edited the patch to change the first two to
          the old name and that worked. How are you generating the patch?
        • The patch shows GeoBaseMembershipShape (newly added) as extending
          GeoBaseShape, yet that can’t be since GeoBaseShape should now be named
          BasePlanetObject. I hand edited it to be BasePlanetObject; but this
          didn't give me a warm and fuzzy that this patch file is ready for me
          to work with. ...
        • GeoCircle.getBounds couldn't figure out how to call super.getBounds
          because there is no superclass implementing that method, just the
          interface.

        Can you provide a better patch Karl? Was your code compiling at the
        time you generated the patch?


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

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

        Show
        Karl Wright added a comment - Perhaps try applying the patch with svn patch?? Sent from my Windows Phone From: David Smiley (JIRA) Sent: 6/27/2015 2:05 AM To: dev@lucene.apache.org Subject: [jira] [Commented] ( LUCENE-6578 ) Geo3d: arcDistanceToShape() method may be useful [ https://issues.apache.org/jira/browse/LUCENE-6578?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14604002#comment-14604002 ] David Smiley commented on LUCENE-6578 : -------------------------------------- Why not support both? The current strategy is eminently extensible, which is what I like about it. Sure; +1. I construed this as a generic "internal distance" method, which is how it's used in my employer's application. Removing the genericism would bust that application. So I prefer to leave this the way it is, thanks. Okay; this can be handled with javadocs that explain how this shape implements computeDistance. I went to apply your patch but ran into some issues in doing so: The patch shows a class was renamed (GeoBaseShape to BasePlanetObject), yet the patch's reference to the file does so in a way that doesn't seem right. The "Index:" line should show the old name, the "---" line should also show the old name, and the "+++" line should show the new name. In your patch, all 3 have the old name. This definitely confused IntelliJ, and I expect it would confuse ReviewBoard too. I hand-edited the patch to change the first two to the old name and that worked. How are you generating the patch? The patch shows GeoBaseMembershipShape (newly added) as extending GeoBaseShape, yet that can’t be since GeoBaseShape should now be named BasePlanetObject. I hand edited it to be BasePlanetObject; but this didn't give me a warm and fuzzy that this patch file is ready for me to work with. ... GeoCircle.getBounds couldn't figure out how to call super.getBounds because there is no superclass implementing that method, just the interface. Can you provide a better patch Karl? Was your code compiling at the time you generated the patch? – This message was sent by Atlassian JIRA (v6.3.4#6332) --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org For additional commands, e-mail: dev-help@lucene.apache.org
        Hide
        Karl Wright added a comment -

        I applied my own patch to a fresh checkout of trunk, and had to add BasePlanetObject by hand afterwards. So then I regenerated this patch; hopefully it works this time.

        Show
        Karl Wright added a comment - I applied my own patch to a fresh checkout of trunk, and had to add BasePlanetObject by hand afterwards. So then I regenerated this patch; hopefully it works this time.
        Hide
        Michael McCandless added a comment -

        Maybe you just need to add the --show-copies-as-adds command-line option when running svn diff?

        Show
        Michael McCandless added a comment - Maybe you just need to add the --show-copies-as-adds command-line option when running svn diff?
        Hide
        Michael McCandless added a comment -

        bq, There are various different ways of compute a distance metric.

        Thanks for the explanation Karl Wright ... it sounds like the added abstraction is indeed warranted.

        Show
        Michael McCandless added a comment - bq, There are various different ways of compute a distance metric. Thanks for the explanation Karl Wright ... it sounds like the added abstraction is indeed warranted.
        Hide
        David Smiley added a comment -

        This is good now; thanks. GeoBaseShape actually stays (it wasn't renamed), so that clarifies my confusion when I tried to fix the patch.

        Show
        David Smiley added a comment - This is good now; thanks. GeoBaseShape actually stays (it wasn't renamed), so that clarifies my confusion when I tried to fix the patch.
        Hide
        Karl Wright added a comment -

        So no need to submit another patch?

        Show
        Karl Wright added a comment - So no need to submit another patch?
        Hide
        David Smiley added a comment -

        Right. I'll post an update tonight hopefully. I worked on it last night some.

        Show
        David Smiley added a comment - Right. I'll post an update tonight hopefully. I worked on it last night some.
        Hide
        David Smiley added a comment -

        FYI I posted to ReviewBoard so that my edits can be seen clearly: https://reviews.apache.org/r/36035/

        One observation I had, perhaps not introduced in this issue, is that GeoPoint.magnitude() should always be 1.0 when using the sphere planet model yet the GeoPoint itself doesn't "know" that. So it goes and computes the Math.sqrt and it's cached. I'm not sure if it's worthwhile, in some issue other than this of course, exploring if GeoPoint should have a unit-sphere subclass different from the non-unit sphere subclass. Just a thought.

        Show
        David Smiley added a comment - FYI I posted to ReviewBoard so that my edits can be seen clearly: https://reviews.apache.org/r/36035/ One observation I had, perhaps not introduced in this issue, is that GeoPoint.magnitude() should always be 1.0 when using the sphere planet model yet the GeoPoint itself doesn't "know" that. So it goes and computes the Math.sqrt and it's cached. I'm not sure if it's worthwhile, in some issue other than this of course, exploring if GeoPoint should have a unit-sphere subclass different from the non-unit sphere subclass. Just a thought.
        Hide
        David Smiley added a comment -

        This is the patch I posted to ReviewBoard, and Karl accepted it. It's mostly about reducing code duplication around duplicate isWithin or some other methods that are overloaded with GeoPoint or Vector. I moved common implementations to base classes, and I added a Java 8 default method. To make back-porting easier I don't leverage the existence of that method; we might change that at a later point. I also eliminated redundant javadocs where I saw them. I also found/fixed some equals/hashcode problems – notably SidedPlane was missing them.

        Suggested CHANGES.txt:

        * LUCENE-6578: Geo3D can now compute the distance from a point to a shape, both
          inner distance and to an outside edge. Multiple distance algorithms are
          available.  (Karl Wright, David Smiley)
        

        I'll commit this later today unless there are objections.

        Show
        David Smiley added a comment - This is the patch I posted to ReviewBoard, and Karl accepted it. It's mostly about reducing code duplication around duplicate isWithin or some other methods that are overloaded with GeoPoint or Vector. I moved common implementations to base classes, and I added a Java 8 default method. To make back-porting easier I don't leverage the existence of that method; we might change that at a later point. I also eliminated redundant javadocs where I saw them. I also found/fixed some equals/hashcode problems – notably SidedPlane was missing them. Suggested CHANGES.txt: * LUCENE-6578: Geo3D can now compute the distance from a point to a shape, both inner distance and to an outside edge. Multiple distance algorithms are available. (Karl Wright, David Smiley) I'll commit this later today unless there are objections.
        Hide
        ASF subversion and git services added a comment -

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

        LUCENE-6578: Geo3D: compute the distance from a point to a shape.

        Show
        ASF subversion and git services added a comment - Commit 1688545 from David Smiley in branch 'dev/trunk' [ https://svn.apache.org/r1688545 ] LUCENE-6578 : Geo3D: compute the distance from a point to a shape.
        Hide
        ASF subversion and git services added a comment -

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

        LUCENE-6578: Geo3D: compute the distance from a point to a shape.

        Show
        ASF subversion and git services added a comment - Commit 1688546 from David Smiley in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1688546 ] LUCENE-6578 : Geo3D: compute the distance from a point to a shape.
        Hide
        David Smiley added a comment -

        FYI during the 5x backport I had to add the isWithin(Vector) implementation to a couple of the classes which had depended on the Java 8 default method. No big deal.

        Thanks for this feature Karl!

        Show
        David Smiley added a comment - FYI during the 5x backport I had to add the isWithin(Vector) implementation to a couple of the classes which had depended on the Java 8 default method. No big deal. Thanks for this feature Karl!
        Hide
        Shalin Shekhar Mangar added a comment -

        Bulk close for 5.3.0 release

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

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development