Geojson styling overhaul

[Conengmo]

I've been thinking about how to decouple the GeoJson data and things like styling. This could reduce the html filesize and prevent unexpected behavior where we edit someones data. I spent some time with the GeoJson class and made some changes:

• Style and highlight is no longer added to the data, but stored in a separate mapping.
  • This mapping is compact, features sharing parameters are defined once.
  • It needs a field to identify a feature with. I made a heuristic function that tries to find one, otherwise tries to add an id field to the data, and throws an error if it fails.
• It is possible to disable embedding the data for files and URLs by setting embed=False.
• It is possible to create the object without loading the data (only for files and URLS with embed=False and no style or highlight).
• Highlighter now works also without style_function. It resets the style on mouseout.
• I made some things explicit, should as the conversion to FeatureCollection.

With better style mappings and an option to disable embedding, html files can become a lot smaller.

Here's a code snippet to try it out with:

highlight_function = lambda x: {'weight': 10 if x['geometry']['type'] == 'MultiPolygon' else 0}

def style_function(feature):
    style = {
        'opacity': 1.0,
        'fillColor': 'green',
        'color': 'black',
        'weight': 2
    }
    if feature['properties']['name'] == 'Alabama':
        style['fillColor'] = 'red'
    if feature['properties']['name'] == 'Utah':
        style['fillColor'] = 'yellow'
    return style

data = 'us-states.json'
# data = 'https://raw.githubusercontent.com/python-visualization/folium/master/tests/us-states.json'
folium.GeoJson(
    data,
    style_function=style_function,
    highlight_function=highlight_function,
    embed=False,
).add_to(m)

@jtbaker do you want to review? I'm wondering if it's a bit too much. Maybe we can simplify it by dropping some functionality. On the other hand, this doesn't increase complexity for the users other than the embed option. Would love to hear what you think about this.

[jtbaker]

This looks really good @Conengmo! I'll find some time this week to do a full review and give some feedback. Thanks for taking the time to think about the functionality and design of this.

I think moving toward a direction of keeping logic and data separate, and having the option of using externally sourced data, can really enable folium to build maps with truly dynamic content.

[jtbaker]

Hey @Conengmo, starting to work my way through this now. I'll leave comments as I go. This a library-wide design pattern I suppose, but for interpolating literals into the JS templates, how would you feel about using Jinja2's tojson method instead of hardcoding in the string quotes around the interpolaters. This would have the added benefit of raising an error (I think) at the python runtime if an object is not JSON serializable.

So this line might look like:

{% for id_val in ids_list %}case {{ id_val | tojson | safe }}: {% endfor %}

Docs on this feature:

http://flask.pocoo.org/docs/0.12/templating/#standard-filters

Let me know what you think. I think it could be a good option to standardize as a sort of consistent style guide across different classes - and allow us to flexibly, but consistently embed dtypes including strings as well as others (numbers, lists, dicts) with less template hardcoding.

[Conengmo]

Hi Jason, nice idea! Let's do this throughout the project. Maybe make our own filters that do the various mappings? Or is tojson suitable for all? If it's okay with you I think we should do that in a separate PR. Maybe you want to open an issue for this?

[jtbaker]

Yes, that sounds great. I'll work in that after we get through getting this merged. I think tojson should be suitable for most, but not for the {{ this._parent.get_name() }} variable embeds and a few others. Will have to be done on a case by case basis - but I think it would be a good standard for string embeds and python dict/list transformations.

[jtbaker]

@Conengmo, sorry for the delay on the review here! I love what you've done and think this is a huge milestone for the project - and that moving to optional embedding may mitigate some of the large notebook rendering issues.

I've left a few comments/suggestions in a few places as a part of this review, but am super excited about this getting merged.

[Conengmo]

The tests were failing because the TimeSliderChoropleth plugin got broken. It integrates with the GeoJson class. I fixed it by making TimeSliderChoropleth no longer inherit from GeoJson. Instead, it lightly reuses some of its components. For that I moved the data processing part of the GeoJson class to a method instead of having it in __init__.

[Conengmo]

I think it's ready, @jtbaker what do you think, okay to merge or do you have more comments?

[jtbaker]

Hey @Conengmo, I gave it another look over and everything looks good to me. I give it the 👍 for the merge and am pretty excited about it. Thanks for the thought put into implementing this feature, I think it will really make the library more performant and while still retain control of the styling python-side.

[Conengmo]

Thanks for the kind words Jason, I appreciate it!

Let’s wait with the merge until 0.8.1 is released, I think this change is big enough to go in 0.9.0.