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

Folium: code should reflect current best practice, latest release #12

Open
emiliom opened this issue Aug 24, 2017 · 6 comments
Open

Folium: code should reflect current best practice, latest release #12

emiliom opened this issue Aug 24, 2017 · 6 comments

Comments

@emiliom
Copy link
Member

emiliom commented Aug 24, 2017

@lsetiawan, in the advanced geopandas notebook, please go over the Folium code and upgrade as needed to ensure it uses current best practices and functionality from the latest Folium release. Pinging @ocefpaf to get his help -- @ocefpaf, maybe you can do that directly? If you're confident you can issue the next Folium release by ~ Sept 4, let's go with code that reflects that release.

My Folium code always seemed clunky to me (having to convert the GeoDataFrame to GeoJSON, and then relying on the obscure knowledge of GeoJSON FeatureCollection structure to properly set key_on="features.properties.pfaf_7"). I'm crossing my fingers that the choropleth plotting method, is now more GeoPandas friendly. Also, the legend wasn't coming out fully as expected -- the title (legend_name) didn't show up, as shown in examples I'd seen.

You two can work on this together as you see fit. FYI, the notebook link above is to the https://github.com/emiliom/geopandas-tutorial-maptime tutorials, b/c that notebook (unlike the GeoHackweek notebook) doesn't have the complexity of the PostGIS database dependency.

@ocefpaf
Copy link

ocefpaf commented Aug 24, 2017

Pinging @ocefpaf to get his help -- @ocefpaf, maybe you can do that directly?

I can do that as soon as I finish the new folium relase.

If you're confident you can issue the next Folium release by ~ Sept 4, let's go with code that reflects that release.

I've been sprinting on folium for 3-days straight and I plan to issue the new release over this weekend.

My Folium code always seemed clunky to me (having to convert the GeoDataFrame to GeoJSON, and then relying on the obscure knowledge of GeoJSON FeatureCollection structure to properly set key_on="features.properties.pfaf_7"). I'm crossing my fingers that the choropleth plotting method, is now more GeoPandas friendly. Also, the legend wasn't coming out fully as expected -- the title (legend_name) didn't show up, as shown in examples I'd seen.

The GeoJSON conversion is internal now, but I need to polish some corners to actually get choropleth to fully work with GeoPandas objects.

PS: I felt ashamed after I promise a new release for SciPy and did not deliver, then FOSS4G... Also did not deliver ir. The final drop was that last Tuesday, during the CEOS webinars, folium was mentioned in all the talks 😱

@emiliom
Copy link
Member Author

emiliom commented Aug 24, 2017

I can do that as soon as I finish the new folium relase.

In that case, let's see if @lsetiawan can do it instead, hopefully by the end of this week. I want to make sure the PR makes it into the next GeoPandas release, which is planned for early next week.

That's all great news on the Folium release, including choropleth and GeoPandas! Good luck with the last sprint!!

@emiliom
Copy link
Member Author

emiliom commented Sep 8, 2017

Let's shift the discussion from emiliom/geopandas-tutorial-maptime#1 to here. See that PR for previous discussions.

Indeed I do not recommend to merge that, specially if the goal is to teach the software API, even though it is a thin wrapper it can be confusing.

@ocefpaf, this tutorial only presents a brief taste of Folium, specially its interaction with GeoPandas (via a choropleth example). The intent is not to teach the Folium API in any detail. So, the wrapper would be distracting and take up time.

@emiliom I am not clear about this:

see if you can pass the index name or something generic or "automated" like that which refereces the index, rather than requiring the user to think about a column name to pass.

What sort of index are you looking for, an integer or the actual column name?

@lsetiawan, I was referring to the index of the geodataframe. In @ocefpaf's example he passes the pfaf_7 column to the key_on argument. But my hunch is that it's unnecessary to pick a column, and it'd be more generic and meaningful (simpler) in the context of this tutorial, to just pass the geodataframe index. I'm just not sure how it would be done, given the key_on syntax apparently requiring something like 'feature.properties.<column name>. So, please look into it.

@lsetiawan
Copy link
Member

@lsetiawan, I was referring to the index of the geodataframe.

Just to be clear, you are looking for the column index correct? not row index? Sorry for asking so much. I am still a bit confused of the index, since that can either be for column or row.

@lsetiawan
Copy link
Member

lsetiawan commented Sep 8, 2017

@emiliom check out Tony's example:

brewmap.choropleth(data=breweries,
                   geo_data=states, 
                   columns = ['STATE', 'COUNT'], # specifies [<key>, <value>] columns in breweries dataframe
                   key_on  = 'feature.properties.STUSPS',  # specifies <key> in states GeoJSON Shapefile, 
                   fill_color='BuPu', fill_opacity=0.9, line_opacity=0.5,
                   threshold_scale = list(range(0, 1200, 200)),
                   legend_name = "Number of Breweries",
                   highlight=True)

He has two different dataframe with different keys but same values. I think that this help to conceptually understanding how the function works in a better way. As I look at our tutorial, I didn't quite get difference between key in columns and key_on. Now I get that his key value will be the column at which the join occurs between the data and the geometric object.

So maybe hydrobas_ww_p7_wasp gdf should only be a df with pfaf_7, pfaf_6, and area_mi2 attributes, leaving out the polygongeom?

@emiliom
Copy link
Member Author

emiliom commented Sep 8, 2017

Ok. Let's set this aside. It's turning out harder to communicate what I mean, and it's not worth all our effort. I'll look into it when I'm back. Thanks.

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

No branches or pull requests

3 participants