Uploaded image for project: 'Lucene - Core'
  1. Lucene - Core
  2. LUCENE-7158

Haversin should use the earth's mean radius, not its max (equitorial)?

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.0, 6.1, master (7.0)
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Across our spatial modules we seem to disagree about the earth's radius when we model it as a sphere.

      I think in our haversin implementation we use equitorial (maximum) radius, but maybe in spatial3d we use the earth's mean radius.

      I think mean makes more sense: the earth is actually a squashed sphere, so it's polar radius is shorter than its equitorial radius.

      I think it's important, when we model the earth as a sphere, that we pick one radius and try to use that one consistently?

      1. LUCENE-7158.patch
        17 kB
        Karl Wright
      2. LUCENE-7158.patch
        14 kB
        Michael McCandless

        Activity

        Hide
        nknize Nicholas Knize added a comment -

        I also propose refactoring SloppyMath.haversin to o.a.l.geo.GeoUtils in core. It's only used for Geo and the reusable WGS constants live in that class.

        Show
        nknize Nicholas Knize added a comment - I also propose refactoring SloppyMath.haversin to o.a.l.geo.GeoUtils in core. It's only used for Geo and the reusable WGS constants live in that class.
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        +1 to use mean earth radius for haversine computations. It is the right value to use.

        Show
        kwright@metacarta.com Karl Wright added a comment - +1 to use mean earth radius for haversine computations. It is the right value to use.
        Hide
        rcmuir Robert Muir added a comment -

        +1 but as Nick suggests, it should be forced to be consistent with e.g. bounding box generation.

        We had the problem of multiple haversins before and using different constants is just as bad. I think if a user does a range query vs sorting vs a lucene expressions, they should not see inconsistencies just because we don't organize things properly.

        To me this is far more important than any particular value the constant may have.

        Show
        rcmuir Robert Muir added a comment - +1 but as Nick suggests, it should be forced to be consistent with e.g. bounding box generation. We had the problem of multiple haversins before and using different constants is just as bad. I think if a user does a range query vs sorting vs a lucene expressions, they should not see inconsistencies just because we don't organize things properly. To me this is far more important than any particular value the constant may have.
        Hide
        mikemccand Michael McCandless added a comment -

        I also propose refactoring SloppyMath.haversin to o.a.l.geo.GeoUtils in core.

        +1, but maybe do this separately?

        Show
        mikemccand Michael McCandless added a comment - I also propose refactoring SloppyMath.haversin to o.a.l.geo.GeoUtils in core. +1, but maybe do this separately?
        Hide
        mikemccand Michael McCandless added a comment -

        Initial patch, however some geo3d tests are angry about the change from 6371009.0 to 6371008.7714 as the mean ... maybe Karl Wright can help fix them?

        A surprising number of places cared greatly about the exact earth's radius

        Show
        mikemccand Michael McCandless added a comment - Initial patch, however some geo3d tests are angry about the change from 6371009.0 to 6371008.7714 as the mean ... maybe Karl Wright can help fix them? A surprising number of places cared greatly about the exact earth's radius
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        Michael McCandless I bet this is just stapling issues. I will be happy to fix them if my git-fu is good enough.

        Show
        kwright@metacarta.com Karl Wright added a comment - Michael McCandless I bet this is just stapling issues. I will be happy to fix them if my git-fu is good enough.
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        Here's an updated patch with the Geo3D tests restapled.

        Show
        kwright@metacarta.com Karl Wright added a comment - Here's an updated patch with the Geo3D tests restapled.
        Hide
        dsmiley David Smiley added a comment -

        +1. I especially like the URL reference to http://earth-info.nga.mil/GandG/publications/tr8350.2/wgs84fin.pdf – it's important to track where our reference data and lifted algorithms come from.
        What do you mean be "restapled"?

        Show
        dsmiley David Smiley added a comment - +1. I especially like the URL reference to http://earth-info.nga.mil/GandG/publications/tr8350.2/wgs84fin.pdf – it's important to track where our reference data and lifted algorithms come from. What do you mean be "restapled"?
        Hide
        kwright@metacarta.com Karl Wright added a comment -

        "Stapling" means changing tests that check for explicit numerical values to check for different ones if the underlying constants or calculations change.

        Show
        kwright@metacarta.com Karl Wright added a comment - "Stapling" means changing tests that check for explicit numerical values to check for different ones if the underlying constants or calculations change.
        Hide
        mikemccand Michael McCandless added a comment -

        Thanks Karl Wright, tests pass for me now; I'll push shortly.

        Show
        mikemccand Michael McCandless added a comment - Thanks Karl Wright , tests pass for me now; I'll push shortly.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit cf7967cc467d9d697d520fcdf92fcdb52f7ddd4e in lucene-solr's branch refs/heads/master from Mike McCandless
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=cf7967c ]

        LUCENE-7158: use the same value (from WGS84) for earth's mean radius when we approximate it as a sphere

        Show
        jira-bot ASF subversion and git services added a comment - Commit cf7967cc467d9d697d520fcdf92fcdb52f7ddd4e in lucene-solr's branch refs/heads/master from Mike McCandless [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=cf7967c ] LUCENE-7158 : use the same value (from WGS84) for earth's mean radius when we approximate it as a sphere
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 4ecfa98bdbb3c00d6ab2b9fb9ebb27ec59031352 in lucene-solr's branch refs/heads/branch_6x from Mike McCandless
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=4ecfa98 ]

        LUCENE-7158: use the same value (from WGS84) for earth's mean radius when we approximate it as a sphere

        Show
        jira-bot ASF subversion and git services added a comment - Commit 4ecfa98bdbb3c00d6ab2b9fb9ebb27ec59031352 in lucene-solr's branch refs/heads/branch_6x from Mike McCandless [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=4ecfa98 ] LUCENE-7158 : use the same value (from WGS84) for earth's mean radius when we approximate it as a sphere
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 48c80f91b8e5cd9b3a9b48e6184bd53e7619e7e3 in lucene-solr's branch refs/heads/branch_6_0 from Mike McCandless
        [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=48c80f9 ]

        LUCENE-7158: use the same value (from WGS84) for earth's mean radius when we approximate it as a sphere

        Conflicts:
        lucene/sandbox/src/test/org/apache/lucene/document/TestLatLonPointDistanceSort.java
        lucene/spatial/src/java/org/apache/lucene/spatial/util/GeoUtils.java
        lucene/spatial/src/test/org/apache/lucene/spatial/util/BaseGeoPointTestCase.java
        lucene/spatial/src/test/org/apache/lucene/spatial/util/TestGeoUtils.java
        lucene/spatial3d/src/java/org/apache/lucene/spatial3d/Geo3DPoint.java
        lucene/spatial3d/src/test/org/apache/lucene/spatial3d/TestGeo3DPoint.java

        Show
        jira-bot ASF subversion and git services added a comment - Commit 48c80f91b8e5cd9b3a9b48e6184bd53e7619e7e3 in lucene-solr's branch refs/heads/branch_6_0 from Mike McCandless [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=48c80f9 ] LUCENE-7158 : use the same value (from WGS84) for earth's mean radius when we approximate it as a sphere Conflicts: lucene/sandbox/src/test/org/apache/lucene/document/TestLatLonPointDistanceSort.java lucene/spatial/src/java/org/apache/lucene/spatial/util/GeoUtils.java lucene/spatial/src/test/org/apache/lucene/spatial/util/BaseGeoPointTestCase.java lucene/spatial/src/test/org/apache/lucene/spatial/util/TestGeoUtils.java lucene/spatial3d/src/java/org/apache/lucene/spatial3d/Geo3DPoint.java lucene/spatial3d/src/test/org/apache/lucene/spatial3d/TestGeo3DPoint.java
        Hide
        hossman Hoss Man added a comment -

        Manually correcting fixVersion per Step #S6 of LUCENE-7271

        Show
        hossman Hoss Man added a comment - Manually correcting fixVersion per Step #S6 of LUCENE-7271

          People

          • Assignee:
            Unassigned
            Reporter:
            mikemccand Michael McCandless
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development