replace GeoJsonStyle by kwarg style_function in GeoJson #266

BibMartin · 2015-11-28T18:37:15Z

To simplify choropleth generation.

You have to provide a function feature -> style which is to me the most generic API.

I need to update Map.geo_json and tests before merge.

ocefpaf · 2015-11-28T19:31:09Z

folium/features.py

@@ -184,22 +184,56 @@ def __init__(self, data):

                    # providing string
                    GeoJson(open('foo.json').read())
+            style_function: function, default None
+                A function mapping a GeoJson Feature to a style dict.
+                Ex: If you GeoJson has the form


ocefpaf · 2015-11-28T19:46:19Z

You have to provide a function feature -> style which is to me the most generic API.

I like that as a low level interface: powerful and simple. We just need to provide some high level methods wrapping that and, as you mention, the choropleth is the first obvious one.

I also think we should comply with the simple-spec by default. That would solve #198 and #120.

@BibMartin if I understood your implementation it would be a matter if writing a style_function for the simple-spec, correct? We can do that later once your implementation is complete. We just need to be clear in the docs that the style_function will expect leaflet defaults, like fillOpacity instead of fill-opacity.

PS: Another killer PR! Thanks!!

themiurgo · 2015-11-30T13:34:58Z

I'm against this as it is now, unless you manage to convince me. 😀

In folium the name GeoJson has always been confused with Choroplets. I think this is a major error and it needs to be fixed soon. Naming this object GeoJsonStyle goes in the opposite direction. Perhaps ChoroplethStyle instead? A hint that this is not just GeoJson is that it has properties like "color domain", "range" and such. GeoJson is not about mapping values to colors.

In the end we want:

To be able to quickly import / export features as GeoJson.
That GeoJson follows simple-spec, as @ocefpaf . Let's look at http://geojson.io for reference. Folium should be the Python version of that, augmented with all the nice plugins it has already.
To be able to generate choroplet, but that should be handled by different methods than the GeoJson import / export.

Let me know what you think about this. 😄

ocefpaf · 2015-11-30T14:11:31Z

In folium the name GeoJson has always been confused with Choroplets. I think this is a major error and it needs to be fixed soon.

Agreed. And we are working on that here.

Naming this object GeoJsonStyle goes in the opposite direction. Perhaps ChoroplethStyle instead?

This PR removes the GeoJsonStyle and adds a kwarg to style the GeoJSON for any purpose and not only choropleth, so I do not think any renaming is necessary.

A hint that this is not just GeoJson is that it has properties like "color domain", "range" and such. GeoJson is not about mapping values to colors.

But that is styling that goes in the ~~Geo~~JSON when plotting.

To be able to quickly import / export features as GeoJson.

That GeoJson follows simple-spec, as @ocefpaf . Let's look at http://geojson.io for reference. Folium should be the Python version of that, augmented with all the nice plugins it has already.

And I believe that this implementation, with a default set to comply with the simple-spec, achieve this goal. What do you think @BibMartin?

To be able to generate choroplet, but that should be handled by different methods than the GeoJson import / export.

Why not just another styling function that we can, and should, add another method that call this class and pass a styling function for easy choropleth generation.

Note that we do not need to force anything beyond the simple-spec as the default, but if people have wonky JSONs styles then have the tool to pass them here.

BibMartin · 2015-11-30T14:52:44Z

With this PR, you can:

Put a GeoJson on the map, with GeoJson(my_geojson).
If my_geojson features have an attribute properties.style expressed in Leaflet style syntax, it will be taken into account on the map. It can be used for choropleth, but also for various reasons.
You can overwrite (update) the attributes properties.style in providing a styling function : GeoJson(my_geojson, my_style_function).
You can use the old Map.geo_json method to draw a choropleth. I'm okay with both of you to rename it choropleth ; but I would take the occasion to change it's API.

@ocefpaf

like that as a low level interface: powerful and simple. We just need to provide some high level methods wrapping that and, as you mention, the choropleth is the first obvious one.

True. I imagine we shall have the GeoJson object for low-level, and a Map.choropleth method for high-level choropleth. I've just kept the Map.geo_json name for backward-compatibility, but I'm okay to move it asap to Map.choropleth.

I also think we should comply with the simple-spec by default. That would solve #198 and #120.
@BibMartin if I understood your implementation it would be a matter if writing a style_function for the simple-spec, correct?

I did not thought about creating a simple-style style_function ; that'd be a great simple way of doing it. But honestly, I'm not comfortable with simple-style because

there are things belonging to mapbox
it does more than just styling (it creates Icons, popups, etc)
it creates another syntax, while Leaflet has it's own

If you're okay, I would at least let the simple-style part for another PR.

ocefpaf · 2015-11-30T15:12:14Z

I did not thought about creating a simple-style style_function ; that'd be a great simple way of doing it. But honestly, I'm not comfortable with simple-style because

there are things belonging to mapbox

Yes, but I believe that the simple-spec is more wide spread than Leaflet's defaults. I see it a lot in many different mapping clients.

it does more than just styling (it creates Icons, popups, etc)

That is desirable sometimes. Like the choropleth stuff: range, color domain, etc 😉

it creates another syntax, while Leaflet has it's own

Maybe that is the real question. What should be folium's default? simple-spec or Leaflet's standards? I am inclined to simple-spec. However, since PR makes it easy to provide styling functions, maybe we can provide the simple-spec function as a built-in option. Not sure 😑

If you're okay, I would at least let the simple-style part for another PR.

Yes. I am OK with that. You already have a rebase on your plate 😜

BibMartin · 2015-11-30T15:20:49Z

it does more than just styling (it creates Icons, popups, etc)

That is desirable sometimes. Like the choropleth stuff: range, color domain, etc 😉

I'm okay, but it means that we cannot support the full spec with this object. We need to have a style object that can contain a Icon, Popup and so on. I'll think about how to do it simply for another PR.

As for this PR, do I break the geo_json method API immediatly in favor of a choropleth ?

ocefpaf · 2015-11-30T15:44:25Z

I'm okay, but it means that we cannot support the full spec with this object. We need to have a style object that can contain a Icon, Popup and so on.

That is fine. I though something along the lines of mapping fillOpacity → fill-opacity, etc, while just ignoring Icons/Popups and other stuff that do not map directly.

Maybe, under that light, the simplespec should not be the default.

We can still implement a simple-spec styling function and provide it as an option. That function would be the first step to adding a simple-spec GeoJSON. And we can think about implementing this mapping style function + some Icon/Pop loop that would make a "simple-spec method." To me that is another 👍 to the robustness of this PR.

BibMartin · 2015-12-01T21:29:56Z

It is (painfully) rebased ; and ready to merge.
Unless you want to take the occasion to replace

def geo_json(...):
    """

by

def geo_json(**kwargs):
    warnings.warn('This method is deprecated. Please use Map.choropleth instead.')
    return self.choropleth(**kwargs)

def choropleth(...):
    """

I can do it before squashing.

ocefpaf · 2015-12-02T12:12:11Z

folium/features.py

@@ -169,7 +169,7 @@ def render(self, **kwargs):


 class GeoJson(MacroElement):
-    def __init__(self, data):
+    def __init__(self, data, style_function=None):


We still need to add a slim version of the simple-spec option. I am OK doing that in another PR. Let's just open an issue to keep track. (Or a new comment on the current issues we have open about the simple-spec.)

ocefpaf · 2015-12-02T12:22:33Z

It is (painfully) rebased

Sorry. I won't wait so long to review typos and pep8 style. (And I should not do that when we have many PRs pending.)

Unless you want to take the occasion to replace

I think we are better off doing that in another PR.

I made a quick review. Nothing important.

Let me know if you want to address those here or if you want me to do those later.

@themiurgo are you in peace with this PR or do you still have some doubts? Your comments are valuable to us!

I am 👍 to merge this PR.

themiurgo · 2015-12-02T12:45:10Z

I'm mainly ok. I'm not 100% ok with the name, but let's include it and see how it work out in the future. 👍

themiurgo · 2015-12-02T12:51:41Z

I did not thought about creating a simple-style style_function ; that'd be a great simple way of doing it. But honestly, I'm not comfortable with simple-style because

there are things belonging to mapbox

it does more than just styling (it creates Icons, popups, etc)

it creates another syntax, while Leaflet has it's own

I hear your concerns, but at the moment simplespec-style is the only standard, the naming used by Leaflet is not a standard, it is tied to their javascript implementation and nobody uses it outside Leaflet. Also, siplespec-style is increasingly used in many places, including GitHub (a non-exaustive list:
https://github.com/mapbox/simplestyle-spec/wiki/Implementations). I think folium needs to be easily extendable and customizable, but have sane defaults. simplespec-style is part of the sane defaults in my opinion.

BibMartin · 2015-12-02T13:22:52Z

Thanks @themiurgo and @ocefpaf for the review.

@ocefpaf

Let me know if you want to address those here or if you want me to do those later.

I'll try to apply changes tonight (CET) and squash in the same time.

ocefpaf · 2015-12-02T14:22:30Z

I'll try to apply changes tonight (CET) and squash in the same time.

No rush. Just ping me once you are done.

themiurgo · 2015-12-04T10:38:14Z

@BibMartin Could you please squash everything, so that I can merge? :)

BibMartin · 2015-12-04T11:10:28Z

Could you please squash everything, so that I can merge? :)

Done.

Three remarks (before @ocefpaf will pep8 this) 😄

My pep8ing, breaking, and scrambling coding skills are everywhere! No code line is safe 👻

Yes, that's why challenging PEP8 before merge is a good habit. Maybe one day I'll be able to write PEP8-complianty code directly. Thanks to both of you for review, and thanks @ocefpaf for 👻

ocefpaf · 2015-12-04T12:35:12Z

@BibMartin can we do something about the None in the colorbar?

ocefpaf · 2015-12-04T12:36:23Z

folium/folium.py

-    def geo_json(self, geo_path=None, geo_str=None, data_out='data.json',
+    def geo_json(self, *args, **kwargs):
+        """This method is deprecated, see `Map.choropleth` instead."""
+        warnings.warn('This method is deprecated. Please use Map.choropleth instead.')


Let's add and will be removed in v0.2.1

ocefpaf · 2015-12-04T12:51:53Z

To deal with the None let's just do

        legend_name: string, default empty string

Copy-and-paste docstring update too 😉

BibMartin · 2015-12-04T12:53:06Z

I had it for None-> "" but forgot about the docstring

ocefpaf · 2015-12-04T12:54:15Z

Let me update my branch and try again. I got that None yesterday.

PS: do you think you can rebase this to get the _repr_html() fix?

BibMartin · 2015-12-04T12:55:43Z

If you're finished, I do an extra commit and wait for your feedback before re-squashing

What repr_html fix ?

ocefpaf · 2015-12-04T12:58:07Z

From #275. Just because I am lazy and it will make it easier to see the results on the notebook.

ocefpaf · 2015-12-04T13:11:52Z

Just re-run all my notebooks that uses choropleths and they look awesome!

@BibMartin great work! And sorry for the last minute burst review. Squash?
@themiurgo Now am I done 😳 I promise no more teasing for a merge and then asking for changes 😜

themiurgo · 2015-12-04T13:56:27Z

It'd be great if this could be squashed again. Sorry @BibMartin you can blame it on @ocefpaf 😜

ocefpaf · 2015-12-04T14:05:29Z

Sorry @BibMartin you can blame it on @ocefpaf 😜

I take all the blame as long as @BibMartin keeps sending awesome PRs 😸

BibMartin · 2015-12-04T14:43:15Z

Squashed (sorry for the 2 hours delay ; I was in a meeting)

themiurgo · 2015-12-04T15:40:46Z

Merge! Congrats to @BibMartin and thanks to @ocefpaf for the comments.

Replace GeoJsonStyle by kwarg style_function in GeoJson

ocefpaf · 2015-12-04T15:45:46Z

🎉

ocefpaf mentioned this pull request Nov 28, 2015

Simpler GeoJsonStyle #264

Closed

ocefpaf reviewed Nov 28, 2015
View reviewed changes

themiurgo mentioned this pull request Nov 30, 2015

The .geo_json() method is broken in master #263

Closed

3 tasks

BibMartin force-pushed the choropleth branch from 39fdb62 to 70700bf Compare December 1, 2015 21:21

ocefpaf reviewed Dec 2, 2015
View reviewed changes

ocefpaf mentioned this pull request Dec 2, 2015

Fix WMS only layer bug #274

Merged

BibMartin mentioned this pull request Dec 2, 2015

Support GeoJSON simplestyle #120

Closed

ocefpaf mentioned this pull request Dec 3, 2015

How about we kill the current way to do choropleth? #255

Closed

BibMartin force-pushed the choropleth branch from ab93fb1 to 623e72a Compare December 4, 2015 11:06

ocefpaf reviewed Dec 4, 2015
View reviewed changes

ocefpaf mentioned this pull request Dec 4, 2015

What are folium natural dependencies ? #241

Closed

BibMartin force-pushed the choropleth branch from 623e72a to 768db9d Compare December 4, 2015 13:03

GeoJsonStyle->kwarg GeoJson.style_function, Map.geo_json->Map.choropleth

01ec8aa

BibMartin force-pushed the choropleth branch from 768db9d to 01ec8aa Compare December 4, 2015 14:40

BibMartin mentioned this pull request Dec 4, 2015

Describe Marker by GeoJSON/TopoJSON #6

Closed

themiurgo closed this Dec 4, 2015

themiurgo reopened this Dec 4, 2015

themiurgo added a commit that referenced this pull request Dec 4, 2015

Merge pull request #266 from BibMartin/choropleth

1366528

Replace GeoJsonStyle by kwarg style_function in GeoJson

themiurgo merged commit 1366528 into python-visualization:master Dec 4, 2015

BibMartin mentioned this pull request Dec 5, 2015

Support for categorical/discrete colour maps in map.geo_json() #89

Closed

BibMartin deleted the choropleth branch December 7, 2015 11:02

BibMartin mentioned this pull request Jan 4, 2016

Data not binding to choropleth in latest folium 0.2.0.dev (but worked in 0.1.6) #316

Closed

ocefpaf added the enhancement Feature request or idea about how to make folium better label Feb 12, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace GeoJsonStyle by kwarg style_function in GeoJson #266

replace GeoJsonStyle by kwarg style_function in GeoJson #266

BibMartin commented Nov 28, 2015

ocefpaf Nov 28, 2015

ocefpaf commented Nov 28, 2015

themiurgo commented Nov 30, 2015

ocefpaf commented Nov 30, 2015

BibMartin commented Nov 30, 2015

ocefpaf commented Nov 30, 2015

BibMartin commented Nov 30, 2015

ocefpaf commented Nov 30, 2015

BibMartin commented Dec 1, 2015

ocefpaf Dec 2, 2015

ocefpaf commented Dec 2, 2015

themiurgo commented Dec 2, 2015

themiurgo commented Dec 2, 2015

BibMartin commented Dec 2, 2015

ocefpaf commented Dec 2, 2015

themiurgo commented Dec 4, 2015

BibMartin commented Dec 4, 2015

ocefpaf commented Dec 4, 2015

ocefpaf Dec 4, 2015

ocefpaf commented Dec 4, 2015

BibMartin commented Dec 4, 2015

ocefpaf commented Dec 4, 2015

BibMartin commented Dec 4, 2015

ocefpaf commented Dec 4, 2015

ocefpaf commented Dec 4, 2015

themiurgo commented Dec 4, 2015

ocefpaf commented Dec 4, 2015

BibMartin commented Dec 4, 2015

themiurgo commented Dec 4, 2015

ocefpaf commented Dec 4, 2015

replace GeoJsonStyle by kwarg style_function in GeoJson #266

replace GeoJsonStyle by kwarg style_function in GeoJson #266

Conversation

BibMartin commented Nov 28, 2015

ocefpaf Nov 28, 2015

Choose a reason for hiding this comment

ocefpaf commented Nov 28, 2015

themiurgo commented Nov 30, 2015

ocefpaf commented Nov 30, 2015

BibMartin commented Nov 30, 2015

ocefpaf commented Nov 30, 2015

BibMartin commented Nov 30, 2015

ocefpaf commented Nov 30, 2015

BibMartin commented Dec 1, 2015

ocefpaf Dec 2, 2015

Choose a reason for hiding this comment

ocefpaf commented Dec 2, 2015

themiurgo commented Dec 2, 2015

themiurgo commented Dec 2, 2015

BibMartin commented Dec 2, 2015

ocefpaf commented Dec 2, 2015

themiurgo commented Dec 4, 2015

BibMartin commented Dec 4, 2015

ocefpaf commented Dec 4, 2015

ocefpaf Dec 4, 2015

Choose a reason for hiding this comment

ocefpaf commented Dec 4, 2015

BibMartin commented Dec 4, 2015

ocefpaf commented Dec 4, 2015

BibMartin commented Dec 4, 2015

ocefpaf commented Dec 4, 2015

ocefpaf commented Dec 4, 2015

themiurgo commented Dec 4, 2015

ocefpaf commented Dec 4, 2015

BibMartin commented Dec 4, 2015

themiurgo commented Dec 4, 2015

ocefpaf commented Dec 4, 2015