-
Notifications
You must be signed in to change notification settings - Fork 14k
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
[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
@@ -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", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🍾 🎆 🍾
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments feel free to address or happy to merge as is
dotRadius={pointRadius} | ||
pointRadiusUnit={pointRadiusUnit} | ||
rgb={rgb} | ||
globalOpacity={globalOpacity} | ||
compositeOperation={'screen'} | ||
renderWhileDragging={renderWhileDragging} | ||
aggregatorName={aggregatorName} | ||
aggregation={customMetric ? aggregatorName : undefined} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: I think we generally prefer null
over undefined
though effectively it's the same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, can change this to null
.
superset/viz.py
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
}); | ||
opts.reduce = (accu, prop) => { | ||
// Temporarily disable param-reassignment linting to work with supercluster's api | ||
/* eslint-disable no-param-reassign */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
d9d4066
to
55a1955
Compare
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.
@mistercrunch @betodealmeida @youngyjd @kristw