[refactor] Remove dependency on personal fork of supercluster from mapbox visualizations #5902

xtinec · 2018-09-14T22:29:41Z

Update dependency to reference the vanilla supercluster
Clean up backend api call for mapbox vizzes to ensure a boolean is sent to indicate whether the viz includes custom metric for clustering
Refactor of mapbox and its cluster overlay components to use vanilla supercluster and its recommended way for handling clustering based on custom aggregations.
Allow reclustering within the initial bounds on render in mapbox visualizations (stay true to old behaviors).
Remove the median aggregation from available cluster label aggregators as there is no memory efficient way to implement this and it is unknown how often this feature is used
Updating doc to mention the backward incompatible change re median

@mistercrunch @betodealmeida @youngyjd @kristw

mistercrunch · 2018-09-17T20:05:18Z

superset/assets/package.json

@@ -124,7 +124,7 @@
    "shortid": "^2.2.6",
    "sprintf-js": "^1.1.1",
    "srcdoc-polyfill": "^1.0.0",
-    "supercluster": "https://github.com/georgeke/supercluster/tarball/ac3492737e7ce98e07af679623aad452373bbc40",
+    "supercluster": "^4.1.1",


🍾 🎆 🍾

mistercrunch

Minor comments feel free to address or happy to merge as is

mistercrunch · 2018-09-17T20:08:14Z

superset/assets/src/visualizations/MapBox/MapBox.jsx

          dotRadius={pointRadius}
          pointRadiusUnit={pointRadiusUnit}
          rgb={rgb}
          globalOpacity={globalOpacity}
          compositeOperation={'screen'}
          renderWhileDragging={renderWhileDragging}
-          aggregatorName={aggregatorName}
+          aggregation={customMetric ? aggregatorName : undefined}


NIT: I think we generally prefer null over undefined though effectively it's the same

Yep, can change this to null.

mistercrunch · 2018-09-17T20:15:15Z

superset/viz.py

@@ -1960,7 +1960,7 @@ def get_data(self, df):
            return None
        fd = self.form_data
        label_col = fd.get('mapbox_label')
-        custom_metric = label_col and len(label_col) >= 1
+        custom_metric = bool(label_col) and len(label_col) >= 1


[optional] we typically don't cast to bool in python, (direct checks on whether a value is truthy are common), but I can see where it's coming from.

custom_metric is serialized and sent to the client and then used to determine how to cluster the incoming data. Without this change, we get [] when using the default point count for aggregation and true when using a particular column (e.g. population). The client code is more readable when we send a proper boolean as custom_metric. I would probably rename this field to has_custom_metric to reflect its type.

Perhaps I should use not label_col here to produce a boolean for the client to consume?

mistercrunch · 2018-09-17T20:16:02Z

superset/assets/src/visualizations/MapBox/MapBox.jsx

+    });
+    opts.reduce = (accu, prop) => {
+      // Temporarily disable param-reassignment linting to work with supercluster's api
+      /* eslint-disable no-param-reassign */


[non actionable] interesting related conversation:
eslint/eslint#8581

…pbox visualizations - Update dependency to reference the vanilla supercluster - Clean up backend api call for mapbox vizzes to ensure a boolean is sent to indicate whether the viz includes custom metric for clustering - Refactor of mapbox and its cluster overlay components to use vanilla supercluster and its recommeded way for handling clustering based on custom aggregations. - Allow reclustering within the initial bounds on render in mapbox visualizations (stay true to old behaviors). - Remove the median aggregation from available cluster label aggregators as there is no memory efficient way to implement this and it is unknown how often this feature is used - Updating doc to mention the backward incompatible change re median

… a boolean and rename the field reflect it is a boolean.

mistercrunch · 2018-09-18T19:38:14Z

LGTM

…pbox visualizations (apache#5902) * [refactor] Remove dependency to personal fork of supercluster from mapbox visualizations - Update dependency to reference the vanilla supercluster - Clean up backend api call for mapbox vizzes to ensure a boolean is sent to indicate whether the viz includes custom metric for clustering - Refactor of mapbox and its cluster overlay components to use vanilla supercluster and its recommeded way for handling clustering based on custom aggregations. - Allow reclustering within the initial bounds on render in mapbox visualizations (stay true to old behaviors). - Remove the median aggregation from available cluster label aggregators as there is no memory efficient way to implement this and it is unknown how often this feature is used - Updating doc to mention the backward incompatible change re median * Perform the check for has_custom_metric through `not None` to produce a boolean and rename the field reflect it is a boolean.

xtinec changed the title ~~[refactor] Remove dependency to personal fork of supercluster from mapbox visualizations~~ [refactor] Remove dependency on personal fork of supercluster from mapbox visualizations Sep 14, 2018

mistercrunch reviewed Sep 17, 2018

View reviewed changes

xtinec added 2 commits September 17, 2018 21:43

Perform the check for has_custom_metric through not None to produce…

55a1955

… a boolean and rename the field reflect it is a boolean.

xtinec force-pushed the xtinec/use-mapbox-supercluster branch from d9d4066 to 55a1955 Compare September 18, 2018 04:44

mistercrunch merged commit 24be692 into apache:master Sep 18, 2018

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[refactor] Remove dependency on personal fork of supercluster from mapbox visualizations #5902

[refactor] Remove dependency on personal fork of supercluster from mapbox visualizations #5902

xtinec commented Sep 14, 2018 •

edited

Loading

mistercrunch Sep 17, 2018

mistercrunch left a comment

mistercrunch Sep 17, 2018

xtinec Sep 17, 2018

mistercrunch Sep 17, 2018

xtinec Sep 17, 2018 •

edited

Loading

mistercrunch Sep 17, 2018

mistercrunch commented Sep 18, 2018

[refactor] Remove dependency on personal fork of supercluster from mapbox visualizations #5902

[refactor] Remove dependency on personal fork of supercluster from mapbox visualizations #5902

Conversation

xtinec commented Sep 14, 2018 • edited Loading

mistercrunch Sep 17, 2018

Choose a reason for hiding this comment

mistercrunch left a comment

Choose a reason for hiding this comment

mistercrunch Sep 17, 2018

Choose a reason for hiding this comment

xtinec Sep 17, 2018

Choose a reason for hiding this comment

mistercrunch Sep 17, 2018

Choose a reason for hiding this comment

xtinec Sep 17, 2018 • edited Loading

Choose a reason for hiding this comment

mistercrunch Sep 17, 2018

Choose a reason for hiding this comment

mistercrunch commented Sep 18, 2018

xtinec commented Sep 14, 2018 •

edited

Loading

xtinec Sep 17, 2018 •

edited

Loading