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

GeoJsonTooltip fails to show pop-up on non-connected areas (e.g. city with islands) #929

Closed
mjkvaak opened this issue Aug 10, 2018 · 17 comments
Labels
bug An issue describing unexpected or malicious behaviour

Comments

@mjkvaak
Copy link

mjkvaak commented Aug 10, 2018

Hi there!
First of all, thank you so much for the excellent work you have done so far with folium!

I played with the folium GeoJson and noticed a problem with the GeoJsonTooltip -function, namely it fails to show a pop-up when moving mouse over an area that is not connected, that is consisting of two (or more) different areas (Polygons). You can see the unwanted behavior in the screen cap I attached with cities having island. The names of the cities consisting of only one connected area (only one Polygon) work as they should. I guess that when generating the pop up the function tries to look for a polygon but fails when it finds a list of them instead.

ezgif-2-599d86b757

@jtbaker
Copy link
Contributor

jtbaker commented Aug 10, 2018

Hey @mjkvaak! Glad you're enjoying it. Are you sure that the polygons in question have a property that matches the fields you've specified? Can you open up the console from your browser's dev tools, and see if it is throwing an error? That would be helpful in trying to debug this issue.

@mjkvaak
Copy link
Author

mjkvaak commented Aug 13, 2018

I now digged deeper into the "bug" and found out that the problems with pop-ups' labels are encountered when the geometry types of non-connected areas equaled to shapely's GeometryCollection. After changing the types into shapely Multipolygon (by running .apply(lambda x: Multipolygon(x)) on the geometry column) the pop-ups worked as they should. GeoJson understood both GeometryCollection and Multipolygon -types in the sense that they both produced the correct areas on the map, but on the former I couldn't get the pop-ups working with the non-connected areas.

@jtbaker
Copy link
Contributor

jtbaker commented Aug 14, 2018

Ok, good to know. Did GeoPandas .to_json() write out to the GeometryCollection spec instead of MultiPolygon?

One thing I did notice when developing the tool is that when sticky is set to False, the tooltip will only display on one of the polygons for a MultiPolygon.

@mjkvaak
Copy link
Author

mjkvaak commented Aug 15, 2018

I'm not sure if I fully understood, but after applying the .to_json() on the GDF the GeometryCollection objects remain as GeometryCollection objects, i.e. are not converted into MultiPolygons.

It may be more convenient for you to download the json-file I am using from GitHub: https://github.com/tomimick/mapcolorizer/blob/master/data-finland/data/kuntarajat.geojson (Borders of Finnish Cities) and playing with it yourself. The below example demonstrates the problem I'm having with the pop-up -tool.

Ps. I forgot to mention earlier, but I'm running on folium version 0.5.0+152.g05f8e81, win 7 and Chrome 68.0.3440.106 (Official build, 64-bit).

import geopandas as gpd
import folium

gdf = gpd.read_file('kuntarajat.geojson').iloc[0:20]

fmap= folium.Map(location=[65,25], 
                    tiles='cartodbpositron',
                    zoom_start=5,
                    control_scale=True)

folium.GeoJson(
        gdf[['name', 'geometry']].to_json(), 
        name='Finnish cities',
        show=True,
        style_function=lambda feature: {
            'fillColor': 'green',
            'color': 'black',
            'weight': 1,
            'dashArray': '5, 5',
            'fillOpacity':0.5
        },   
        highlight_function=lambda x: {'weight':3, 
                                      'color':'black',
                                      'fillOpacity':1},    
        tooltip=folium.features.GeoJsonTooltip(
            fields=['name'],
            aliases=['City name:'],                              
        ),  
).add_to(fmap)

display(fmap)```

@jtbaker
Copy link
Contributor

jtbaker commented Aug 15, 2018

Ok, thanks for the info! I tested it and see what's happening.

Did a little more research and found this and this. It seems like the best practice right now is to use the Multi-geometries when possible instead of of the GeometryCollections - most use cases for GeometryCollection, I think could/should warrant their own properties key, instead of mixing them into a singular feature.

Leaflet's .bindTooltip function is being passed one argument, layer which doesn't seem to be traversing this data structure properly with layer.feature.properties as is configured right now. Leaflet is reading all the individual polygons in the Geometry Collection as individual features, and looking out one nested layer for a properties key that is in fact two layers of nesting out.

I think this could possibly be accounted for in our tool, but I'm not sure how common it is to encounter these Geometry Collections in practice or if it is more of a unicorn. @Conengmo, @ocefpaf, /anyone else, have you guys come across these?

Thanks for pointing out this bug and bringing to our attention, @mjkvaak!

@Conengmo
Copy link
Member

Good that you already dived into this problem. I see how GeometryCollection should be rare, but may be misused more often as we see here.

Is it even possible for us to fix this, or are we dependent on Leaflet? You're description suggests we cannot. Maybe unless we define a separate tooltip case for Geometry collection.

Maybe the best/easiest fix is to do an assertion on the type when someone is adding tooltips. I think it's not unreasonable to have tooltips not working when someone wants to use a rare type.

@jtbaker
Copy link
Contributor

jtbaker commented Aug 15, 2018

I did a little testing on it and was able to work around the limitation using the .onEachFeature key in the L.geoJson() callback object.

Check it out here: jsfiddle

Adapting to this (it seems like?) would preclude the usage of our separate class templating structure in folium, though, since I don't think we can access onEachFeature outside of the callback, like we can .bindTooltip, .bindPopup etc. - so I don't think {{ this.parent.get_name() }}[dot][operation] chained templating model would work.

This makes me think of something else though: toggling permanent: true (JS line 13 in the fiddle) in the .bindTooltip method displays all the tooltips at once, instead of on hovering. This works because they are bound to the individual features instead of the layer as a whole, when they are bound at the onEachFeature level.

This could be a good solution for map labeling. I think folium.Tooltip(text='A String',permanent=True) should work like that for individual vectors/markers, how people might be using DivIcon right now, but for folium.GeoJson, perhaps a labels arg could be useful to label features on a property in the geoJson. I know this would make the GeoJson template a little more crowded though. Thoughts?

@Demetrio92
Copy link
Contributor

Demetrio92 commented Aug 31, 2018

@mjkvaak what is your problem, but thanks a lot for tooltip=folium.features.GeoJsonTooltip -- this made my day. I spent a lot of time looking thought the docs for how to use it.


Btw, if you post your code here, it can be easily formatted using

```python
CODE_HERE
\```  -- without "\"

E.g.:

fmap= folium.Map(
    location=[65,25],
    tiles='cartodbpositron',
    zoom_start=5,
    control_scale=True
)

folium.GeoJson(
    gdf[['name', 'geometry']].to_json(),
    name='Finnish cities',
    show=True,
    style_function=lambda feature: {
        'fillColor': 'green',
        'color': 'black',
        'weight': 1,
        'dashArray': '5, 5',
        'fillOpacity':0.5
    },
    highlight_function=lambda x: {'weight':3,
        'color':'black',
        'fillOpacity':1
    },
    tooltip=folium.features.GeoJsonTooltip(
        fields=['name'],
        aliases=['City name:'],
    ),
).add_to(fmap)

can anyone add the above to the examples?...

@mjkvaak
Copy link
Author

mjkvaak commented Sep 3, 2018

@Demetrio92 I agree that it can take some effort to understand how the GeoJson tooltips work and it was very difficult to find examples in the interwebs. Glad to hear that the above code could help you!

@Conengmo
Copy link
Member

@jtbaker I like the idea of having permanent working with GeoJson tooltips. But it's not really a solution for this issue I think. This idea should be a separate issue/PR. Maybe you can open a new one for it? We could perhaps do something with the render function in GeoJson and have it collect any GeoJsonTooltip children so you can use the onEachFeature function.

For this issue to me it seems we have three options:

  1. Detect GeometryCollection and convert it to MultiPolygon.
  2. Detect GeometryCollection and warn about possible issues with tooltips.
  3. Leave it as it is. Assuming it's an edge case, people encountering it can look up this issue.

I'm not a fan of 1, because it may introduce new problems. 2 seems the least risky. I would be okay with 3, since though this issue is a bug, things are not broken, just not as expected.

@jtbaker and @mjkvaak, could you chime in on what action we should take? I want to work towards closing this issue.

@Conengmo Conengmo added the bug An issue describing unexpected or malicious behaviour label Sep 29, 2018
@mjkvaak
Copy link
Author

mjkvaak commented Sep 29, 2018

@Conengmo I'd go for option 2. Also option 3 sounds OK, since both the issue and the solution have been covered here.

@Conengmo
Copy link
Member

@mjkvaak thanks for chiming in. Do you want to open a PR for this?

I'm now thinking we could check if there is a GeometryCollection and if one of the children is a GeoJsonTooltip and then raise an exception or warning. Most warnings get surpressed so I wonder how useful that is.

If you don't have the time that's fine as well, we'll go with option 3 :)

@mjkvaak
Copy link
Author

mjkvaak commented Oct 12, 2018

@Conengmo Thanks for the offer, but I'm afraid I am not experienced enough coder to commit changes to this package. What you suggested about the warning msg sounds good, thinking that there might be people for whom the unexpected behaviour goes otherwise unnoticed (which can be a bit nasty in some situations). That is, better to give a friendly warning suggesting to use something else than GeometryCollection.

@jtbaker
Copy link
Contributor

jtbaker commented Oct 13, 2018

@Conengmo I took a stab at some QA on this. The problem is, if we place this in the render function, like below:

    def render(self, **kwargs):
        """Renders the HTML representation of the element."""
        if isinstance(self._parent, GeoJson):
            print(self._parent)
            keys = tuple(self._parent.data['features'][0]['properties'].keys())

            # My new code starts here
            geom_collections = [feature['id'] for feature in self._parent.data['features'] if feature['geometry'][
                'type'] == 'GeometryCollection']
            if len(geom_collections) != 0:
                raise ValueError("GeoJsonTooltip is not configured to render tooltips for GeoJson GeometryCollection "
                                 "geometries. Please consider reworking feature IDs {0} to "
                                 "MultiPolygon".format(', '.join(geom_collections)))
            # And ends here.

        elif isinstance(self._parent, TopoJson):
            obj_name = self._parent.object_path.split('.')[-1]
            keys = tuple(self._parent.data['objects'][obj_name][
                             'geometries'][0]['properties'].keys())
        else:
            raise TypeError('You cannot add a GeoJsonTooltip to anything else '
                            'than a GeoJson or TopoJson object.')
        keys = tuple(x for x in keys if x not in ('style', 'highlight'))
        for value in self.fields:
            assert value in keys, ("The field {} is not available in the data. "
                                   "Choose from: {}.".format(value, keys))
        super(GeoJsonTooltip, self).render(**kwargs)

the ValueError doesn't display because the render function is overwriting the content in the out cell in Jupyter. I could rework it as an assertion, but that would break execution and we just want it to be a warning. GeoJsonTooltip can only access self._parent inside of the render() function, right? Any thoughts? I don't think this is major pressing issue, but it could help other people that stumble onto it further down the line.

@Conengmo
Copy link
Member

Looks good what you're doing. I don't really understand what the problem is, if you raise a ValueError that should work, also in Jupyter. It should stop executing there and show the error. What am I missing?

@jtbaker
Copy link
Contributor

jtbaker commented Oct 13, 2018

I don't think it should be a breaking QC check though - just give the user a warning that the GeometryCollection feature tooltips will not display as intended, and still build the rest of the map content - like @mjkvaak's example above.

@Conengmo
Copy link
Member

Good point. How about a UserWarning then? Those get through, I just checked.

warnings.warn('hi', RuntimeWarning)

@Conengmo Conengmo added the work in progress Work is in progress on a PR, check the PR to see its status label Oct 15, 2018
@Conengmo Conengmo removed the work in progress Work is in progress on a PR, check the PR to see its status label Jul 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue describing unexpected or malicious behaviour
Projects
None yet
Development

No branches or pull requests

4 participants