Choropleth take gdf

[ocefpaf]

Ping @emiliom who is interested in this feature.

See it in action here http://nbviewer.jupyter.org/gist/ocefpaf/9ecac736938af0c09b8f14a3850b11fb

I am refactoring the GeoJSON reader too, but I guess I'll stop now so we can have a new release.

[stickler-ci]

C901 'Map.choropleth' is too complex (17)

[emiliom]

Quick comment -- I'm in a rush.

From your example:

m.choropleth(
    geo_data=quartiers,
    data=quartiers,
    columns=['objectid', 'number_bike_stations_relative'],
    key_on='feature.properties.objectid',
    fill_color='BuGn',
    highlight=True
)

This isn't a big improvement, I think 😿. With a GeoDataFrame as the input, it's not necessary to have separate geo_data, data and key_on arguments; they all can come from the same object, the GeoDataFrame ("quartiers"), so only one argument is needed instead of those 3. Plus key_on='feature.properties.objectid' has always been overly complex -- it comes from the GeoJSON structure, and it doesn't apply to a GeoDataFrame.

Maybe you should push the choropleth enhancement to the next release, so as not to delay you further??

[ocefpaf]

This isn't a big improvement, I think 😿. With a GeoDataFrame as the input, it's not necessary to have separate geo_data, data and key_on arguments; they all can come from the same object, the GeoDataFrame ("quartiers"), so only one argument is needed instead of those 3. Plus key_on='feature.properties.objectid' has always been overly complex -- it comes from the GeoJSON structure, and it doesn't apply to a GeoDataFrame.

One step at a time 😉

This PR only factors out the geojson parser to a canonical place instead of parsing it here and later again in the GeoJSON class.

However, to implement what you request we would need a complex API to cover all the possibilities.
One alternative that I am playing with is to make choropleth a class and add methods like .from_geopandas, from_geojson, etc.

The reason I don't want to change this function is b/c the geojson+df is the most primitive combination that we can support. I cannot add geopandas as a hard dependency.

[emiliom]

Got it. To be clear, I wasn't suggesting the other arguments be dropped. I was just trying to clarify that when both geo_data and data come from the same GeoDataframe, key_on ideally should not be required (let alone having it be specified as the GeoJSON-derived, complex 'feature.properties.objectid').

Having key_on not be required for GeoDataframe and geo_data and data point to the same GeoDataFrame would already be a nice and sufficient improvement (in my book) for this new release. Refactoring choropleth as a class could then be left for the next release ;)

Thanks!

[stickler-ci]

C901 'Map.choropleth' is too complex (17)

[ocefpaf]

To be clear, I wasn't suggesting the other arguments be dropped.

It is not about dropping. We cannot make a complex API that takes both the primitive and the high level.
I am tempted to kill the primitive though. One can always use the raw GeoJSON interface to do the same. So the choropleth would be a high level shortcut. I'll sleep on it.

[emiliom]

Worth mentioning that this PR addresses issue #499

[ocefpaf]

This isn't a big improvement, I think 😿.

I discussed this with other folium devs and we are leaning against not implementing a full fledged geopandas support. Not b/c we don't like geopandas, we love it, but b/c we want to keep folium's API as simple as possible and let people extend it to their needs.

Having key_on not be required for GeoDataframe

We would always need to know the column to "keyon" the data b/c that is not standardized. In this example it is named objectid, in other is is only id. Sometimes that is nested inside a deeper level than just feature.properties...

and geo_data and data point to the same GeoDataFrame would already be a nice and sufficient improvement (in my book) for this new release.

That can be easily wrapped and, If we assume that the id is always inside the feature.properties as it is usually expected, one can write a thin wrapper like the one in cell [5] of this notebook:

http://nbviewer.jupyter.org/urls/gist.githubusercontent.com/ocefpaf/839693baecbf9f9e7a02f475bb29b71c/raw/91af232960ee9352c052ca6373f471165c48f59f/geopandas_demo.ipynb

That takes the geodataframe, the id to keyon, and the data column. Not sure we can reduce this further without becoming very specific to the dataframe in question.

[emiliom]

@ocefpaf, no worries. The decisions are all understandable. I'm excited to see you finally issued a new folium release, a few hours ago. Congratulations!!

Thanks for the sample wrapper. I think I can simplify it even further; I'll let you know.

FYI, @lsetiawan and I will be asking you for help on the new release of Folium this week ;) To make sure we and our fellow GeoHackweek tutors are using it to its full potential, and using current best practices.

[ocefpaf]

I think I can simplify it even further; I'll let you know.

Yep. The more you can assume about the underlying data the more you can simplify that. I sent a PR to your repository with that example.

@lsetiawan and I will be asking you for help on the new release of Folium this week ;) To make sure we and our fellow GeoHackweek tutors are using it to its full potential, and using current best practices.

No problem. There will be bugs 😄