Adding icon_create_function argument to Fast Marker Cluster plugin

[Gordonei]

Overview

The Fast Marker Cluster plugin is great, especially with the latest changes making it easier to pass in additional data for the markers. A missing feature is the ability to pass in a custom JavaScript function for generating the cluster icons.

To keep things simple, I've copied over the interface from MakerCluster, and I'm passing through any function created transparently. The only substantive change required was updating the Fast Marker's Jinja template.

Testing

Linting

$ flake8 folium/plugins/fast_marker_cluster.py --max-line-length=120
$ flake8 tests/plugins/fast_marker_cluster.py --max-line-length=120

Unit Tests

$ python3 -m pytest tests
========================================= test session starts ==========================================
platform linux -- Python 3.6.7, pytest-4.3.1, py-1.8.0, pluggy-0.9.0
rootdir: /home/gordon/workspace/folium, inifile: setup.cfg
plugins: xdist-1.27.0, forked-1.0.2, flake8-1.0.4, cov-2.6.1, nbval-0.9.1
collected 134 items                                                                                    

tests/test_features.py .........                                                                 [  6%]
tests/test_folium.py ................                                                            [ 18%]
tests/test_jinja.py ..............                                                               [ 29%]
tests/test_map.py .......                                                                        [ 34%]
tests/test_raster_layers.py ....                                                                 [ 37%]
tests/test_repr.py .....                                                                         [ 41%]
tests/test_utilities.py .................................................                        [ 77%]
tests/test_vector_layers.py ......                                                               [ 82%]
tests/plugins/test_antpath.py .                                                                  [ 82%]
tests/plugins/test_beautify_icon.py .                                                            [ 83%]
tests/plugins/test_boat_marker.py ..                                                             [ 85%]
tests/plugins/test_dual_map.py .                                                                 [ 85%]
tests/plugins/test_fast_marker_cluster.py ....                                                   [ 88%]
tests/plugins/test_feature_group_sub_group.py .                                                  [ 89%]
tests/plugins/test_float_image.py .                                                              [ 90%]
tests/plugins/test_fullscreen.py .                                                               [ 91%]
tests/plugins/test_heat_map.py ...                                                               [ 93%]
tests/plugins/test_heat_map_withtime.py .                                                        [ 94%]
tests/plugins/test_marker_cluster.py .                                                           [ 94%]
tests/plugins/test_minimap.py .                                                                  [ 95%]
tests/plugins/test_pattern.py .                                                                  [ 96%]
tests/plugins/test_polyline_text_path.py .                                                       [ 97%]
tests/plugins/test_scroll_zoom_toggler.py .                                                      [ 97%]
tests/plugins/test_terminator.py .                                                               [ 98%]
tests/plugins/test_time_slider_choropleth.py x                                                   [ 99%]
tests/plugins/test_timestamped_geo_json.py .                                                     [100%]

=========================================== warnings summary ===========================================
/home/gordon/.local/lib/python3.6/site-packages/pkg_resources/__init__.py:1151
/home/gordon/.local/lib/python3.6/site-packages/pkg_resources/__init__.py:1151
tests/test_folium.py::TestFolium::test_choropleth_features
tests/test_folium.py::TestFolium::test_choropleth_features
tests/test_folium.py::TestFolium::test_choropleth_features
tests/test_folium.py::TestFolium::test_choropleth_features
tests/test_folium.py::TestFolium::test_choropleth_features
tests/test_folium.py::TestFolium::test_choropleth_features
tests/test_folium.py::TestFolium::test_choropleth_features
tests/test_folium.py::TestFolium::test_choropleth_features
tests/test_folium.py::TestFolium::test_choropleth_features
  /home/gordon/.local/lib/python3.6/site-packages/pkg_resources/__init__.py:1151: DeprecationWarning: Use of .. or absolute path in a resource path is not allowed and will raise exceptions in a future release.
    self, resource_name

-- Docs: https://docs.pytest.org/en/latest/warnings.html
========================= 133 passed, 1 xfailed, 11 warnings in 43.97 seconds ==========================

Ad-hoc Tests

Defining a JS function that determines the cluster class based upon the content of its children's popup content:

function (cluster) {
   var childCount = cluster.getChildCount();
   var childMarkers = cluster.getAllChildMarkers();
   
   // Working out proportion of children whose popup content starts with "+"
   var withinProportion = 0.0;
   for (i = 0; i < childCount; i++) {
     var popup = childMarkers[i].getPopup();
     var content = popup.getContent();
     if (content[0] == "+") {
       withinProportion += 1.0;
     }
   }
   withinProportion /= childCount;
   
   // Working out class based upon proportion
   var c = ' marker-cluster-';
   if (withinProportion > 0.9) {
     c += 'small';
   } 
   else if (withinProportion > 0.8) {
     c += 'medium';
   } 
   else {
     c += 'large';
   }
   
   return L.divIcon({ html: '<div><span>' + cluster.getChildCount() + '</span></div>' , 
                     className: 'marker-cluster' + c,
                     iconSize: new L.Point(40, 40)});
};

Function is saved as a multiline string in icon_create.

Passing into the Marker cluster:

fast_marker_cluster = FastMarkerCluster(
    data=data,
    callback=callback,
    icon_create_function=icon_create
)

Resulting map with clusters customised based on child content:

[Conengmo]

Hi @Gordonei, thanks for your PR! I agree on this feature missing in FastMarkerCluster when compared to MarkerCluster, let's add it.

Your PR already looks good, but I don't really like that we're duplicating the MarkerCluster template in the FastMarkerCluster template. Looking at your PR got me thinking whether it's not better to have FastMarkerCluster have MarkerCluster as an attribute instead of inheriting of it.

In pseudo code:

class FastMarkerCluster(MacroElement):
    _template = """
        {{ this.marker_cluster.render() or something }}
        for row in {{ this.data|tojson }}:
            marker(data).add_to({{ marker_cluster.get_name() }}

    def __init__(self, stuff, more_stuff):
        self.marker_cluster = MarkerCluster(stuff)
        self.data = more_stuff

What do you think? Want to look into this? If not that's okay, your PR is mergable as is so we can also refactor later.

[Gordonei]

Thanks for your work on this great library!

My initial hunch is that I would expect FastMarkerCluster to inherit from MarkerCluster, being a specialisation on its behaviour of wrapping the Leaflet plugin, but I think you have a point about rather viewing the marker cluster as an attribute to keep the code DRY.

The other option I was considering was sticking with inheritance, but modifying the MarkerCluster template so that FastMarkerCluster doesn't need its own template, but I do worry it would over complicate the MarkerCluster template.

Let me think about it for a few days - I'll only be able to do serious coding on Wednesday again, anyway. Should we maybe merge this in the meanwhile to get the functionality in, and we do another PR for the refactor?

[Conengmo]

Should we maybe merge this in the meanwhile to get the functionality in, and we do another PR for the refactor?

Sure! The API will not change with the refactor, so we can merge this change already.

Let me think about it for a few days

Sounds good, I'm interested what you come up with.