Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use correct ECEF coordinates #58

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rhodes73
Copy link

  • Fix bug in geodesic to ECEF coordinate conversion function
  • Re-enable use of ECEF coordinates in KD-Tree

As pointed out in #10, when using ECEF coordinates without this fix for the latitude in the z-coordinate, some lookups result in wrong results:

In [1]: import reverse_geocoder as rg
   ...: rg.search([(37.78674, -122.39222)])
Loading formatted geocoded file...
Out[1]:
[OrderedDict([('lat', '37.77993'),
              ('lon', '-121.97802'),
              ('name', 'San Ramon'),
              ('admin1', 'California'),
              ('admin2', 'Contra Costa County'),
              ('cc', 'US')])]

After the fix, this does not happen anymore:

In [1]: import reverse_geocoder as rg
   ...: rg.search([(37.78674, -122.39222)])
Loading formatted geocoded file...
Out[1]:
[OrderedDict([('lat', '37.77493'),
              ('lon', '-122.41942'),
              ('name', 'San Francisco'),
              ('admin1', 'California'),
              ('admin2', 'San Francisco County'),
              ('cc', 'US')])]

- Correct bug in geodesic to ECEF coordinate conversion function
- Re-enable use of ECEF coordinates in KD-Tree
@nttams
Copy link

nttams commented Sep 14, 2022

Hi,
I'm also looking for a solution to use ECEF in KDTree. This PR looks ok, why has not it been merged yet?
Thanks.

@Dobatymo
Copy link

This PR might finally fix this library, after being broken for most of the time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants