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

Room for improvement on directions interface #65

Closed
xoolive opened this issue Jun 25, 2016 · 13 comments · Fixed by #91
Closed

Room for improvement on directions interface #65

xoolive opened this issue Jun 25, 2016 · 13 comments · Fixed by #91

Comments

@xoolive
Copy link
Contributor

xoolive commented Jun 25, 2016

(from #60)

Some room for improvements, here are some reflections:

  • It would be great to be able to pass as data either lat/lon tuples or the kind of dictionaries returned by googlemaps API.
import googlemaps
import gmaps

client = googlemaps.Client(">>>> fill in your API key <<<<")
location = client.geocode("Toulouse")
print (location) # have a look!
gmaps.add_layer(Directions(data=[client.geocode("Toulouse"), client.geocode("Paris")]))
  • Also, we hardcoded the travelmode parameter which could be passed as parameter (though I have no idea how to pass something more than data). More generally, it would be great to have a close interface to googlemaps and write:
# from googlemaps documentation
directions_result = client.directions("Sydney Town Hall",
                                     "Parramatta, NSW",
                                     mode="transit",
                                     departure_time=now)
# how about this ?
gmaps.add_layer(Directions("Sydney Town Hall",
                           "Parramatta, NSW",
                           mode="transit",
                           departure_time=now))

For future references, there is a good overview of the interface here.

@pbugnion
Copy link
Owner

pbugnion commented Jun 26, 2016

For anyone interested in tackling adding parameters, here is how to get started. To add the ability to set the travel mode, for instance:

  • Add a traitlet that sets the travel mode to the Python model:
class Directions(widgets.Widget):
    ...
    travel_mode = Enum(
        values=["walking", "driving"], default_value="driving"
    ).tag(sync=True)

This allows you to write:

```python
d = Directions(data=[...], travel_mode="walking")
  • read the value of this traitlet in the render method in the JavaScript view:
var DirectionsLayerView = GMapsLayerView.extend({

    render: function() {

        ...
        var travel_mode = this.get_travel_mode();

        var request = {
            origin: orig,
            destination: dest,
            waypoints: wps,
            travelMode: travel_mode
        };
        ...
    },
    ...
    get_travel_mode: function() {
        var model_travel_mode = this.model.get("travel_mode");
        var mapping = {
            "walking": google.maps.DirectionsTravelMode.WALKING,
            "driving": google.maps.DirectionsTravelMode.DRIVING
        }
        return mapping[model_travel_mode];
    }
});

This tells the view to read the travel_mode traitlet from the model, rather than having it hard-coded.

Adding things like departure time etc. should be similarly easy.

@pbugnion
Copy link
Owner

Adding parameters addresses half of the problem. The other half -- automatic geocoding to translate from string addresses to latitude / longitude pairs -- is a little different. I think a set of translation functions to inter-operate with the googlemaps module would work best, but I don't have strong opinions on this yet.

@xoolive
Copy link
Contributor Author

xoolive commented Jun 26, 2016

Note to myself: a reasonable interface to start working on would be

gmaps.add_layer(Directions(data=[client.geocode("Toulouse"), client.geocode("Paris")]))

@pbugnion
Copy link
Owner

I think I disagree with this interface somewhat. I would rather keep the data array as latitude / longitude pairs, since that's the lowest common denominator. We could then provide helper functions that help with the geocoding. The interface could look like:

import gmaps

gmaps.configure(API KEY)

journey = ["Toulouse", "Paris"]
data = [gmaps.geocode(location) for location in journey]
# data is then an array like [[43.6, 1.2], [48.0, 2.3]]
d = Directions(data=data, travel_mode="driving")

... where geocode is a function in gmaps that relies on the googlemaps module for the geocoding, but returns a single latitude / longitude pair.

This lets people do their own geocoding if they want. Any thoughts? If there is something to be gained by passing the entire googlemaps.geocode result to Directions, I'll happily change my mind.

@xoolive
Copy link
Contributor Author

xoolive commented Jun 27, 2016

Oh actually, the result of gmaps.geocode() is a list of big fat dictionaries with the lat/lon pair hidden deep inside, so I thought that accepting this kind of dictionary would be reasonable.

>>> client.geocode("Paris")
[{'address_components': [{'long_name': 'Paris',
    'short_name': 'Paris',
    'types': ['locality', 'political']},
   {'long_name': 'Paris',
    'short_name': 'Paris',
    'types': ['administrative_area_level_2', 'political']},
   {'long_name': 'Île-de-France',
    'short_name': 'Île-de-France',
    'types': ['administrative_area_level_1', 'political']},
   {'long_name': 'France',
    'short_name': 'FR',
    'types': ['country', 'political']}],
  'formatted_address': 'Paris, France',
  'geometry': {'bounds': {'northeast': {'lat': 48.9021449, 'lng': 2.4699209},
    'southwest': {'lat': 48.815573, 'lng': 2.225193}},
   'location': {'lat': 48.856614, 'lng': 2.3522219},
   'location_type': 'APPROXIMATE',
   'viewport': {'northeast': {'lat': 48.9021449, 'lng': 2.4699209},
    'southwest': {'lat': 48.815573, 'lng': 2.225193}}},
  'place_id': 'ChIJD7fiBh9u5kcRYJSMaMOCCwQ',
  'types': ['locality', 'political']},
 {'address_components': [{'long_name': 'Paris',
    'short_name': 'Paris',
    'types': ['locality', 'political']},
   {'long_name': 'Lamar County',
    'short_name': 'Lamar County',
    'types': ['administrative_area_level_2', 'political']},
   {'long_name': 'Texas',
    'short_name': 'TX',
    'types': ['administrative_area_level_1', 'political']},
   {'long_name': 'United States',
    'short_name': 'US',
    'types': ['country', 'political']}],
  'formatted_address': 'Paris, TX, USA',
  'geometry': {'bounds': {'northeast': {'lat': 33.7383781, 'lng': -95.435455},
    'southwest': {'lat': 33.6118529, 'lng': -95.627928}},
   'location': {'lat': 33.6609389, 'lng': -95.55551299999999},
   'location_type': 'APPROXIMATE',
   'viewport': {'northeast': {'lat': 33.7383781, 'lng': -95.435455},
    'southwest': {'lat': 33.6118529, 'lng': -95.627928}}},
  'place_id': 'ChIJmysnFgZYSoYRSfPTL2YJuck',
  'types': ['locality', 'political']},
# truncated
]

@pbugnion
Copy link
Owner

The results of googlemaps.geocode is indeed a big dictionary, but I'm suggesting creating our own geocode method that just returns a latitude-longitude pair. The body could be something like:

def geocode(location):
    """
    Return a (latitude, longitude) pair for a location.

    >>> geocode("paris")
    (48.856614, 2.4699209)
    """
    googlemaps_geocode = client.geocode(location) # client is a googlemaps.Client instance
    googlemaps_location = googlemaps_geocode[0]["geometry"]["location"]
    latitude = googlemaps_location["latitude"]
    longitude = googlemaps_location["longitude"]
    return (latitude, longitude)

(I haven't tested this method)

The results returned by our geocode method could be fed straight into a data array.

@xoolive
Copy link
Contributor Author

xoolive commented Jun 27, 2016

Looks fair. 😉

@reyale
Copy link

reyale commented Jul 2, 2016

I have mixed feelings on the lat-lon pairs, as structured data is probably going to give it to you in two arrays.

import pandas as pd
df = pd.read_csv('./data.csv')

#would rather:
layer = gmaps.Heatmap(lat=df.lat, lon=data.lon)

#than:
result = [(k,v) for k,v in zip(df.lat, df.lon)]
layer = gmaps.Heatmap(data=result))

m.add_layer(layer)

Will allow for more seamless use with numpy.array as well.

@xoolive
Copy link
Contributor Author

xoolive commented Jul 2, 2016

I don't have any strong opinions on that matter but as a matter of fact, it is common for me to handle a list of structured data (dict or object with accessors to lat/lon) indexed by time.

Well, in the end it is always not free to project the data into the proper interface...

@reyale
Copy link

reyale commented Jul 2, 2016

Well, in the end it is always not free to project the data into the proper interface...

Yeah, but the objective is to remain as close to the most common presentation of the data as possible so that the amount of conversions is reduced. I guess I'm making the case that databases, csv files, and other structured data is going to (generally) give you back everything in vectorized arrays. Maybe you can make the case that the geocoders always think in the other representation (dicts, tuples, ect), but I don't have the experience to comment on that.

@pbugnion
Copy link
Owner

pbugnion commented Jul 3, 2016

This is a great discussion!

The original motivation for gmaps was to help explore geographical data. For this use case, most users will have data in arrays that are built from databases or CSV files, as @reyale said. However, I'm not convinced this makes an interface like Heatmap(lat=[...], lng=[...]) necessarily the right choice. After all, something like:

import pandas as pd
df = pd.read_csv(...)
Heatmap(data=df[["latitude", "longitude"]])

... is just as obvious. Clearly latitude and longitude belong together and passing in one without the other doesn't make much sense. Note that this doesn't work at the moment, but fixing it is easy and doesn't change the interface (see issue #66).

I can see the case for having split lat=, lng= keyword arguments but I don't think the added benefit (if any) really justify the breaking API changes etc, as well as the extra development work that this would entail. I'd much rather concentrate on adding new layers and adding the ability to customise existing layers than on changing the API.

@reyale
Copy link

reyale commented Jul 3, 2016

I'm not convinced this makes an interface like Heatmap(lat=[...], lng=[...]) necessarily the right choice.

I think we're missing the plot a bit here. the lat= lng= interface may not be the correct function prototype (I'm just as happy with the data= to be honest). What I'm getting at is that you don't want to have to do the common reshape:[(k,v) for k,v in zip(df.lat, df.lon)] in application space. If the underlying google api requires tuples (lat,lon) the shaping from[lats,lons] to [(lat,lon), ...] (essentially column-wise concat) should be done in the library. This gives the user an api that most directly interacts with his common presentation of the data. Essentially we're moving common reshape functionality in to the library which can be optimized away later (if it becomes a bottle-neck). This greatly reduces duplicated application layer code.

So if you like the data= I suggest the interface/logic to be something like:

Layer(data=[vector,vector])

class Layer:
    def __init__(self, data, ...):
        self.lat_long_tuples = self._reshape(data)

    #may be unnecessary, depends on the google api
    def _reshape(self,data):
        return [(k,v) for k,v in zip(data[0], data[1])]

This allows behavior like:

import pandas as pd
df = pd.read_csv('data.csv')
result = df[(df['lat'] > 40) & (df['lon'] > 40)]
Layer(data=[result['lat'],result['lon']])

to just work.

Sorry for all the armchair commentary. I'll roll up my sleeves and contribute soon!

@pbugnion
Copy link
Owner

pbugnion commented Jul 4, 2016

Contributions are definitely welcome!

Since the shape of the data array is only loosely related to the directions interface, I've opened issue #68 and we should move the discussion there.

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

Successfully merging a pull request may close this issue.

3 participants