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

Add of_address classmethod to EarthLocation #5154

Merged
merged 8 commits into from Jul 7, 2016

Conversation

Projects
None yet
6 participants
@adrn
Member

adrn commented Jul 3, 2016

In the same vein as SkyCoord.from_name(), this PR implements a from_address() classmethod on EarthLocation that queries the Google maps API to get a latitude and longitude for a specified address. As with Google maps, the address can be a fully specified address (e.g., 123 Main St., New York, NY), or a city (e.g., Danbury), or a country, etc. etc...

cc @eteq @bmorris3

@adrn adrn changed the title from Added from_address classmethod to EarthLocation to Add from_address classmethod to EarthLocation Jul 3, 2016

@bmorris3

This comment has been minimized.

Contributor

bmorris3 commented Jul 3, 2016

Awesome! I have used that API before too, and accidentally hit the query limit without intending to do a huge number of queries. Would you be open to a PR to your branch that adds a little cache, as one way to prevent users from hitting the query limit?

@mhvk

This comment has been minimized.

Contributor

mhvk commented Jul 4, 2016

This looks very nice. Can the google API also return a height?

@adrn

This comment has been minimized.

Member

adrn commented Jul 4, 2016

@mhvk There is a separate API for retrieving elevations -- I implemented an option to also query and get that as well. @bmorris3 this would require a separate cache if you still think a cache would be useful

See Also
--------
https://developers.google.com/maps/documentation/geocoding/intro

This comment has been minimized.

@mhvk

mhvk Jul 4, 2016

Contributor

These lines cause (I think) the sphinx build to fail. Maybe listing them as references will help? I.e.,

References
----------
.. [1] https://developers.google.com/maps/documentation/geocoding/intro
.. [2] https://developers.google.com/maps/documentation/elevation/intro
This is intended as a quick convenience function to get fast access to
locations. In the background, this just issues a query to the Google maps
geocoding API. It is not meant to be abused! Google uses IP-based query

This comment has been minimized.

@mhvk

mhvk Jul 4, 2016

Contributor

If using references, you could add [1]_ here (and delete the last sentence).

NY) or a city name (e.g., Danbury, CT), or etc.
get_height : bool (optional)
Use the retrieved location to perform a second query to the Google maps
elevation API to retrieve the height of the input address.

This comment has been minimized.

@mhvk

mhvk Jul 4, 2016

Contributor

And add [2]_ here.

results = resp_data.get('results', [])
if len(results) == 0:

This comment has been minimized.

@MSeifert04

MSeifert04 Jul 4, 2016

Contributor

PEP8 recommends if not results to check for an empty list.

@pllim pllim added this to the v1.3.0 milestone Jul 5, 2016

@mhvk

This comment has been minimized.

Contributor

mhvk commented Jul 6, 2016

@adrn - this looks great. The coverage failure likely is because you do not test the exceptions very thoroughly -- but I think this is fine since the ones you do not test are for time-out and "unknown reason".

One question: should this be of_address in analogy with of_site? Of course, one can also argue by analogy with from_geodetic and from_geocentric, but it shares with of_site that it is an indirect construction. cc @eteq

Also, how will people find out about this? But writing this makes me realize EarthLocation is not very much exposed at all right now -- which perhaps is not so bad as it might be sensible to refactor it somewhat (see #4261). Maybe still raise a new issue?

@adrn

This comment has been minimized.

Member

adrn commented Jul 7, 2016

@mhvk I'm not sure why we have both from_ and of_ methods -- in my own code I always use from_ so I don't get confused -- but I switched the name to make it more like of_site. I also added some documentation in coordinates/index so hopefully that helps with exposure!

@astrofrog

This comment has been minimized.

Member

astrofrog commented Jul 7, 2016

If we're deciding on conventions, I agree that from_ is better and more standard across packages.

@adrn

This comment has been minimized.

Member

adrn commented Jul 7, 2016

@astrofrog OK, do you think I should keep this as from_address and make a separate issue to change of_site to from_site?

@astrofrog

This comment has been minimized.

Member

astrofrog commented Jul 7, 2016

@adrn - that's my personal opinion, but i'm not a coordinates maintainer :)

Google maps, this works with fully specified addresses, location names, city
names, and etc.::
>>> EarthLocation.of_address('1002 Holy Grail Court, St. Louis, MO') # doctest: +REMOTE_DATA +FLOAT_CMP

This comment has been minimized.

@mhvk

mhvk Jul 7, 2016

Contributor

Yay! the Holy Grail!

@mhvk

This comment has been minimized.

Contributor

mhvk commented Jul 7, 2016

Writing the various forms out:

EarthLocation.from_geodetic(10*u.deg, 10.*u.deg)
EarthLocation.from_address('New York')
Earthlocation.of_site('keck')
Earthlocation.of_address('New York')

I think I start very slightly liking the of_address form better (and even more the of_site. One way to distinguish the two might be that the from_? form also has (or at least in principle could have) to_? form, while the of_? does not.

@mhvk

This comment has been minimized.

Contributor

mhvk commented Jul 7, 2016

Also had a quick final review: good that you found a place for it in the documentation!

Unless I hear anything from @eteq, I'll merge this by the end of the day.

@mhvk mhvk self-assigned this Jul 7, 2016

@mhvk mhvk changed the title from Add from_address classmethod to EarthLocation to Add of_address classmethod to EarthLocation Jul 7, 2016

@mhvk mhvk added the Affects-dev label Jul 7, 2016

@mhvk mhvk merged commit 33026ae into astropy:master Jul 7, 2016

2 of 3 checks passed

coverage/coveralls Coverage decreased (-0.03%) to 77.147%
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mhvk

This comment has been minimized.

Contributor

mhvk commented Jul 7, 2016

Thanks, @adrn!

@astrofrog

This comment has been minimized.

Member

astrofrog commented Aug 9, 2016

I think I start very slightly liking the of_address form better (and even more the of_site. One way to distinguish the two might be that the from_? form also has (or at least in principle could have) to_? form, while the of_? does not.

FWIW I still think from_ was better because it's the 'conventional' way to name class methods that do this kind of thing...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment