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

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

Merged
merged 2 commits into from
Sep 18, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions UPDATING.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,13 @@ assists people when migrating to a new version.
dashboards through an automated db migration script. We
do recommend that you take a backup prior to this migration.

* Superset 0.28 deprecates the `median` cluster label aggregator for mapbox visualizations. This particular aggregation is not supported on mapbox visualizations going forward.

## Superset 0.27.0
* Superset 0.27 start to use nested layout for dashboard builder, which is not
* Superset 0.27 start to use nested layout for dashboard builder, which is not
backward-compatible with earlier dashboard grid data. We provide migration script
to automatically convert dashboard grid to nested layout data. To be safe, please
take a database backup prior to this upgrade. It's the only way people could go
to automatically convert dashboard grid to nested layout data. To be safe, please
take a database backup prior to this upgrade. It's the only way people could go
back to a previous state.


Expand All @@ -38,15 +40,15 @@ The PRs bellow have more information around the breaking changes:
* [4565](https://github.com/apache/incubator-superset/pull/4565) : we've
changed the security model a bit where in the past you would have to
define your authentication scheme by inheriting from Flask
App Builder's
App Builder's
`from flask_appbuilder.security.sqla.manager import SecurityManager`,
you now have to derive Superset's
own derivative `superset.security.SupersetSecurityManager`. This
can provide you with more hooks to define your own logic and/or defer
permissions to another system as needed. For all implementation, you
simply have to import and derive `SupersetSecurityManager` in place
of the `SecurityManager`
* [4835](https://github.com/apache/incubator-superset/pull/4835) :
* [4835](https://github.com/apache/incubator-superset/pull/4835) :
our `setup.py` now only pins versions where required, giving you
more latitude in using versions of libraries as needed. We do now
provide a `requirements.txt` with pinned versions if you want to run
Expand Down
2 changes: 1 addition & 1 deletion superset/assets/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Member

Choose a reason for hiding this comment

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

🍾 🎆 🍾

"underscore": "^1.8.3",
"urijs": "^1.18.10",
"viewport-mercator-project": "^5.0.0"
Expand Down
1 change: 0 additions & 1 deletion superset/assets/src/explore/controls.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -1361,7 +1361,6 @@ export const controls = {
'mean',
'min',
'max',
'median',
'stdev',
'var',
]),
Expand Down
74 changes: 39 additions & 35 deletions superset/assets/src/visualizations/MapBox/MapBox.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,6 @@ class MapBox extends React.Component {
height,
}).fitBounds(bounds);
const { latitude, longitude, zoom } = mercator;
// Compute the clusters based on the bounds. Again, this is only done once because
// we don't update the clusters as we pan/zoom in the current design.
const bbox = [bounds[0][0], bounds[0][1], bounds[1][0], bounds[1][1]];
this.clusters = this.props.clusterer.getClusters(bbox, Math.round(zoom));

this.state = {
viewport: {
Expand Down Expand Up @@ -80,10 +76,19 @@ class MapBox extends React.Component {
pointRadiusUnit,
renderWhileDragging,
rgb,
hasCustomMetric,
bounds,
} = this.props;
const { viewport } = this.state;
const isDragging = viewport.isDragging === undefined ? false :
viewport.isDragging;

// Compute the clusters based on the original bounds and current zoom level. Note when zoom/pan
// to an area outside of the original bounds, no additional queries are made to the backend to
// retrieve additional data.
const bbox = [bounds[0][0], bounds[0][1], bounds[1][0], bounds[1][1]];
const clusters = this.props.clusterer.getClusters(bbox, Math.round(viewport.zoom));

return (
<MapGL
{...viewport}
Expand All @@ -98,14 +103,14 @@ class MapBox extends React.Component {
isDragging={isDragging}
width={width}
height={height}
locations={Immutable.fromJS(this.clusters)}
locations={Immutable.fromJS(clusters)}
dotRadius={pointRadius}
pointRadiusUnit={pointRadiusUnit}
rgb={rgb}
globalOpacity={globalOpacity}
compositeOperation={'screen'}
renderWhileDragging={renderWhileDragging}
aggregatorName={aggregatorName}
aggregation={hasCustomMetric ? aggregatorName : null}
lngLatAccessor={(location) => {
const coordinates = location.get('geometry').get('coordinates');
return [coordinates.get(0), coordinates.get(1)];
Expand All @@ -119,34 +124,10 @@ class MapBox extends React.Component {
MapBox.propTypes = propTypes;
MapBox.defaultProps = defaultProps;

function createReducer(aggregatorName, customMetric) {
if (aggregatorName === 'sum' || !customMetric) {
return (a, b) => a + b;
} else if (aggregatorName === 'min') {
return Math.min;
} else if (aggregatorName === 'max') {
return Math.max;
}
return function (a, b) {
if (a instanceof Array) {
if (b instanceof Array) {
return a.concat(b);
}
a.push(b);
return a;
}
if (b instanceof Array) {
b.push(a);
return b;
}
return [a, b];
};
}

function mapbox(slice, payload, setControlValue) {
const { formData, selector } = slice;
const {
customMetric,
hasCustomMetric,
geoJSON,
bounds,
mapboxApiKey,
Expand All @@ -170,18 +151,41 @@ function mapbox(slice, payload, setControlValue) {
return;
}

const clusterer = supercluster({
const opts = {
radius: clusteringRadius,
maxZoom: DEFAULT_MAX_ZOOM,
metricKey: 'metric',
metricReducer: createReducer(aggregatorName, customMetric),
});
};
if (hasCustomMetric) {
opts.initial = () => ({
sum: 0,
squaredSum: 0,
min: Infinity,
max: -Infinity,
});
opts.map = prop => ({
sum: prop.metric,
squaredSum: Math.pow(prop.metric, 2),
min: prop.metric,
max: prop.metric,
});
opts.reduce = (accu, prop) => {
// Temporarily disable param-reassignment linting to work with supercluster's api
/* eslint-disable no-param-reassign */
Copy link
Member

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

accu.sum += prop.sum;
accu.squaredSum += prop.squaredSum;
accu.min = Math.min(accu.min, prop.min);
accu.max = Math.max(accu.max, prop.max);
/* eslint-enable no-param-reassign */
};
}
const clusterer = supercluster(opts);
clusterer.load(geoJSON.features);

ReactDOM.render(
<MapBox
width={slice.width()}
height={slice.height()}
hasCustomMetric={hasCustomMetric}
aggregatorName={aggregatorName}
clusterer={clusterer}
globalOpacity={globalOpacity}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
} from '../../utils/common';

const propTypes = {
aggregation: PropTypes.string,
locations: PropTypes.instanceOf(Immutable.List).isRequired,
lngLatAccessor: PropTypes.func,
renderWhileDragging: PropTypes.bool,
Expand All @@ -35,6 +36,30 @@ const contextTypes = {
isDragging: PropTypes.bool,
};

const computeClusterLabel = (properties, aggregation) => {
const count = properties.get('point_count');
if (!aggregation) {
return count;
}
if (aggregation === 'sum' || aggregation === 'min' || aggregation === 'max') {
return properties.get(aggregation);
}
const sum = properties.get('sum');
const mean = sum / count;
if (aggregation === 'mean') {
return Math.round(100 * mean) / 100;
}
const squaredSum = properties.get('squaredSum');
const variance = (squaredSum / count) - Math.pow(sum / count, 2);
if (aggregation === 'var') {
return Math.round(100 * variance) / 100;
} else if (aggregation === 'stdev') {
return Math.round(100 * Math.sqrt(variance)) / 100;
}
// fallback to point_count, this really shouldn't happen
return count;
};

class ScatterPlotGlowOverlay extends React.Component {
constructor(props) {
super(props);
Expand Down Expand Up @@ -90,34 +115,14 @@ class ScatterPlotGlowOverlay extends React.Component {
const mercator = new ViewportMercator(props);
const rgb = props.rgb;
const clusterLabelMap = [];
let maxLabel = -1;

props.locations.forEach(function (location, i) {
if (location.get('properties').get('cluster')) {
let clusterLabel = location.get('properties').get('metric')
? location.get('properties').get('metric')
: location.get('properties').get('point_count');

if (clusterLabel instanceof Immutable.List) {
clusterLabel = clusterLabel.toArray();
if (props.aggregatorName === 'mean') {
clusterLabel = d3.mean(clusterLabel);
} else if (props.aggregatorName === 'median') {
clusterLabel = d3.median(clusterLabel);
} else if (props.aggregatorName === 'stdev') {
clusterLabel = d3.deviation(clusterLabel);
} else {
clusterLabel = d3.variance(clusterLabel);
}
}

clusterLabel = isNumeric(clusterLabel)
? d3.round(clusterLabel, 2)
: location.get('properties').get('point_count');
maxLabel = Math.max(clusterLabel, maxLabel);
clusterLabelMap[i] = clusterLabel;
clusterLabelMap[i] = computeClusterLabel(location.get('properties'),
props.aggregation);
}
}, this);
const maxLabel = Math.max(...Object.values(clusterLabelMap));

ctx.save();
ctx.scale(pixelRatio, pixelRatio);
Expand Down
10 changes: 2 additions & 8 deletions superset/assets/yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -7114,7 +7114,7 @@ just-extend@^3.0.0:
version "3.0.0"
resolved "https://registry.yarnpkg.com/just-extend/-/just-extend-3.0.0.tgz#cee004031eaabf6406da03a7b84e4fe9d78ef288"

kdbush@^1.0.0, kdbush@^1.0.1:
kdbush@^1.0.1:
version "1.0.1"
resolved "https://registry.yarnpkg.com/kdbush/-/kdbush-1.0.1.tgz#3cbd03e9dead9c0f6f66ccdb96450e5cecc640e0"

Expand Down Expand Up @@ -11732,18 +11732,12 @@ supercluster@^2.3.0:
dependencies:
kdbush "^1.0.1"

supercluster@^4.0.1:
supercluster@^4.0.1, supercluster@^4.1.1:
version "4.1.1"
resolved "https://registry.yarnpkg.com/supercluster/-/supercluster-4.1.1.tgz#cf13c3b28a3fb3db5290bfad7f524e244bd4ce78"
dependencies:
kdbush "^2.0.1"

"supercluster@https://github.com/georgeke/supercluster/tarball/ac3492737e7ce98e07af679623aad452373bbc40":
version "2.1.0"
resolved "https://github.com/georgeke/supercluster/tarball/ac3492737e7ce98e07af679623aad452373bbc40#3ac2d8efc0224fb6f4332a7192b619ded4b967a6"
dependencies:
kdbush "^1.0.0"

supports-color@3.1.2, supports-color@3.1.x:
version "3.1.2"
resolved "https://registry.yarnpkg.com/supports-color/-/supports-color-3.1.2.tgz#72a262894d9d408b956ca05ff37b2ed8a6e2a2d5"
Expand Down
6 changes: 3 additions & 3 deletions superset/viz.py
Original file line number Diff line number Diff line change
Expand Up @@ -1960,9 +1960,9 @@ 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
has_custom_metric = label_col is not None and len(label_col) > 0
metric_col = [None] * len(df.index)
if custom_metric:
if has_custom_metric:
if label_col[0] == fd.get('all_columns_x'):
metric_col = df[fd.get('all_columns_x')]
elif label_col[0] == fd.get('all_columns_y'):
Expand Down Expand Up @@ -2003,7 +2003,7 @@ def get_data(self, df):

return {
'geoJSON': geo_json,
'customMetric': custom_metric,
'hasCustomMetric': has_custom_metric,
'mapboxApiKey': config.get('MAPBOX_API_KEY'),
'mapStyle': fd.get('mapbox_style'),
'aggregatorName': fd.get('pandas_aggfunc'),
Expand Down