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

Adding popup_attribute to the GeoJson feature #376

Closed
wants to merge 10 commits into from

Conversation

om-henners
Copy link

I've made a change to the GeoJson feature to add a popup_attribute parameter. Including this changes the template so that attribute of the feautre is used as the popup for that feature. Based on the map I did for the #gistribe 15 minute map competition.

The popup_attribute takes an attribute name and uses the attribute as the popup value for each feature in the layer.

Signed-off-by: Henry Walshaw <henry.walshaw@gmail.com>
Update to the GeoJson and GeoPandas examples to include the popup attribute

Signed-off-by: Henry Walshaw <henry.walshaw@gmail.com>
@BibMartin
Copy link
Contributor

Awesome! Thanks @om-henners .

First reactions :

  • I'd have liked to be able to provide Popup objects to the function. This can be done in another PR.
    But it would be great if the popup_attribute arg were replaced by a popup_function (at the cost of having popup_function = lambda x: 'my_attribute') to simplify this second step.
  • I would prefer if the if self.popup_attribute: were replaced by {% if this.popup_attribute %} in the template, so that useser may change their mind and set popup_attibute manually after the end of the __init__ method.
  • Having a test would be useful.
  • This is really great and would answer @mattwg's question in Popups on Choropleth Map #375.

@om-henners
Copy link
Author

@BibMartin Happy to put in the full popup support, and you're right - it would be better to have the attribute in a single template. Cheers for the feedback!

Update the geojson popup attribute to being a popup function which will build a _popupContent attribute on features if required.

Signed-off-by: Henry Walshaw <henry.walshaw@gmail.com>
Test on the attribute being correct and the geojson producing correctly, and test on the function creating the correct _popupContent attribue on the feature.

Also update test_geo_json_str so that it includes the empty options parameters.

Signed-off-by: Henry Walshaw <henry.walshaw@gmail.com>
Allow passing the popup_function through the choropleth shortcut on the map object (assuming the dataset is GeoJson)

Signed-off-by: Henry Walshaw <henry.walshaw@gmail.com>
Update the notebooks to include a popup function for GeoJson and GeoDataFrames

Signed-off-by: Henry Walshaw <henry.walshaw@gmail.com>
Signed-off-by: Henry Walshaw <henry.walshaw@gmail.com>
@om-henners
Copy link
Author

@BibMartin I've made a coupe of updates to the pull request:

  • The popup_attribute is now a popup_function parameter - if you pass in a function it should take a GeoJson feature and return a string value that is stored in a new _popupContent parameter in the feature properties. This is calculated when you intialise the GeoJson class. If you pass a string it acts as before, and simply uses that attribute as the popup value.
  • I've put the {% if this.popup_attribute %} in the template as requested, so the user can update the popup_attribute at a later stage. Since it calculates the results of the function on __init__ though setting a function here won't work.
  • I've added tests for the geojson to test_folium.py
  • I've added the popup_function parameter to the choropleth method on the map object as well so it will pass straight through

I also took a look at adding the Popup object, but I'll have to have a bit more of a play with it. First though I'd like to update the TopoJson methods so it's consistent with the GeoJson and also has a popup_function available.

self.popup_attribute = popup_function
elif isinstance(popup_function, binary_type):
self.popup_attribute = text_type(popup_function, 'utf8')

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really great ! Thanks.

I would prefer to move this paragraph (346-358) into style_data and simply write self.popup_function = popup_function here. That way, the following code will also work:

g = GeoJson(data)
g.popup_function = some_function

@ocefpaf
Copy link
Member

ocefpaf commented Mar 3, 2016

Wow! Thanks @om-henners!!

@BibMartin Can I assign you to continue reviewing and merge?

@om-henners
Copy link
Author

@ocefpaf no worries - happy to help.

@BibMartin Happy to move the function to the style method. If it's ok I'll delay to tomorrow and do it on the weekend.

@BibMartin
Copy link
Contributor

@BibMartin Can I assign you to continue reviewing and merge?

sure you can (and you do) ; I'm looking forward to review this.

@BibMartin Happy to move the function to the style method. If it's ok I'll delay to tomorrow and do it on the weekend.

No worry, no hurry. This WE is soon enough 😉

The actual population of the `_popupContent` attribute based on popup_function is now in the style attribute. This means that the `popup_function` and `popup_attribute` can be populated after the Geojson creation.

The `popup_function` is actually a feature property now to ensure that the `popup_attribute` is set correctly when the function is set after the fact.

Signed-off-by: Henry Walshaw <henry.walshaw@gmail.com>
Update the geojson examples to show the popup attribute and function can be set after the fact on geojson features.

Signed-off-by: Henry Walshaw <henry.walshaw@gmail.com>
@om-henners
Copy link
Author

I've updated so that the popup_function and popup_attribute can be set after the fact, and the popup_function is called from within the style_data method.

Fix the indent to fix the comment that fails the travis build

Signed-off-by: Henry Walshaw <henry.walshaw@gmail.com>
@om-henners
Copy link
Author

Hi @BibMartin , any more changes you'd like on this request?

feature["properties"]["_popupContent"] = self.popup_function(feature) # noqa
except TypeError:
pass

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need a try ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the checks on the function are outside the render operation, I was being a bit slack and letting the function fail when it wasn't a function. I'll move checks inside the loop.

@BibMartin
Copy link
Contributor

Hi @om-henners ,

sorry for the delay ; I've been quite busy this fortnight.

I've put a few question in the code but nothing's blocking. I wait for your reactions before merging.

Thnaks again!

@om-henners
Copy link
Author

Hi @BibMartin , likewise sorry for the delay - been super busy here too :) I've added replies above. Happy to make the updates to the code this weekend.

@AlexArcPy
Copy link

Looking forward to see this functionality merged and pushed into conda package :) I believe there is no existing functionality that would let supply a geoJSON property as a pop-up value, yet this is kind of basic thing for most web mapping operations. @om-henners, brilliant.

@ocefpaf
Copy link
Member

ocefpaf commented Aug 1, 2016

@AlexArcPy thanks for the ping.

@om-henners not sure if I will have time, but if I do are you OK if I pick your commits and finish this PR?

@AlexArcPy
Copy link

@ocefpaf, just realized you are the "ocean guy". Your blog posts are such huge time savers for me, every time I find your posts, I was so glad you spend hours writing them, you are legend.

@ocefpaf
Copy link
Member

ocefpaf commented Aug 1, 2016

@ocefpaf, just realized you are the "ocean guy". Your blog posts are such huge time savers for me, every time I find your posts, I was so glad you spend hours writing them, you are legend.

😳

But here on folium the "legend" is @BibMartin 😉

@cloud05
Copy link

cloud05 commented Sep 5, 2016

Good day! I would just like to know when this new feature (popup for geojson) would be implemented. So I guess I'll just leave this here to be notified whenever it is ready. Thank you for your work btw. Cheers!

@ocefpaf ocefpaf force-pushed the master branch 7 times, most recently from 475a974 to 2b3b5d5 Compare September 5, 2017 17:58
@MRigal
Copy link

MRigal commented Sep 8, 2017

Good luck @ocefpaf ! Don't hesitate to ask for help in this refactoring. It might simply be an open PR with the planned changes as comments/pseudo-code...

@ocefpaf ocefpaf force-pushed the master branch 2 times, most recently from a352df2 to 2a2dc9a Compare September 9, 2017 17:09
@pmains
Copy link
Contributor

pmains commented Sep 28, 2017

What is the status of this merge?

@bitofbreeze
Copy link

I would very much like to know as well. Can we please approve this, fixing the conflicts that are now there of course.

@ocefpaf
Copy link
Member

ocefpaf commented Oct 6, 2017

@CSFlorin folium is maintained by volunteers on their free time. So "piling on" an issue won't help.
If you want to try a PR I will gladly review it 😉

@bitofbreeze
Copy link

bitofbreeze commented Oct 8, 2017

side note: couldn't get @halfdanrump 's method to work (map just wouldn't show at all, even when the gdf has just one row), so here's an alternative that just puts a new centered point in each polygon:

def add_points(mapobj, gdf, popup_field_list):
    # Create empty lists to contain the point coordinates and the point pop-up information
    coords, popups = [], [] 
    # Loop through each record in the GeoDataFrame
    for i, row in gdf.iterrows():
        # Append lat and long coordinates to "coords" list
        coords.append([row.geometry.y, row.geometry.x])
        # Create a string of HTML code used in the IFrame popup
        # Join together the fields in "popup_field_list" with a linebreak between them
        label = '<br>'.join([field + ': ' + str(row[field]) for field in popup_field_list])
        # Append an IFrame that uses the HTML string to the "popups" list 
        popups.append(IFrame(label, width = 300, height = 100))
    
    # Create a Folium feature group for this layer, since we will be displaying multiple layers
    pt_lyr = folium.FeatureGroup(name = 'pt_lyr')

    # Add the clustered points of crime locations and popups to this layer
    pt_lyr.add_child(MarkerCluster(locations = coords, popups = popups))

    # Add this point layer to the map object
    mapobj.add_child(pt_lyr)
    return mapobj

gdf['geometry'] = gdf.apply(lambda z: Point(z.geometry.centroid.x, z.geometry.centroid.y), axis=1)
m = add_points(m, gdf, ['cols whose info you want displayed for each polygon'])
m

Sorry, I spoke too soon. I think it wasn't working for me since props was a list, so I joined each element and it worked all of a sudden. Thank you!

@bitofbreeze
Copy link

Does anyone know how to group the layers together so that the popup layer is always above the polygon layer and so that they appear as one in the layercontrol? Much appreciated and sorry for the bugging on this thread

@ghandic
Copy link
Contributor

ghandic commented Oct 18, 2017

This should be solved after GeoJsonCss has been accepted in PR #748 as it allows popups per feature in the GeoJson as well as styling.

@MattGraz
Copy link

I'm a bit unclear as to what the status of this is? Can someone elaborate a bit on what the holdups are (aside from the obvious conflicts)?

I'm happy to put some time into this and get this feature added to Folium. Some guidance on whether or not this pull is salvageable and/or I should start from scratch would be appreciated.

Thanks!

@ghandic
Copy link
Contributor

ghandic commented Jan 21, 2018 via email

@MattGraz
Copy link

@ghandic: Thanks for the follow up. I did get it working! I was just unsure what the status was as far as merging. Thank you for clarifying!

@ritu99
Copy link

ritu99 commented Apr 21, 2018

Any updates?

@Conengmo Conengmo added work in progress Work is in progress on a PR, check the PR to see its status and removed work in progress Work is in progress on a PR, check the PR to see its status labels Apr 22, 2018
@nittolese
Copy link

@Conengmo could you resolve the conflicts in merging the pull request?
Thank you!

@ocefpaf
Copy link
Member

ocefpaf commented May 10, 2018

@Conengmo could you resolve the conflicts in merging the pull request?

We are going in a different direction with GeoJsonCss. Closing this to avoid confusion.

@ocefpaf ocefpaf closed this May 10, 2018
@ocefpaf ocefpaf reopened this May 10, 2018
----------
func : srting, function or None
The popup value for the feature
* If string, then an attribute to use as a popup value for the feature

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E501 line too long (82 > 79 characters)

@ocefpaf ocefpaf closed this May 10, 2018
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

Successfully merging this pull request may close these issues.

None yet