DECKGL integration - Phase 1 #3771

mistercrunch · 2017-11-03T15:38:07Z

Adding a new set of geospatial visualizations building on top of the
awesome deck.gl library. https://github.com/uber/deck.gl

While the end goal it to expose all types of layers and let users bind
their data to control most props exposed by the deck.gl API, this
PR focusses on a first set of visualizations and props:

ScatterLayer
HexagonLayer
GridLayer
ScreenGridLayer

This is a large PR, so here's an overview of what is in here:

a common DeckGLContainer that we'll use to render all deck related visualizations
4 new vizes
an example dash loaded as part of superset load_examples
a new FixedOrMetricControl allowing to provide a fixed value or binding to a metric
...

xrmx · 2017-11-03T15:40:56Z

superset/cli.py

@@ -133,9 +133,15 @@ def load_examples(load_test_data):
    print("Loading [Misc Charts] dashboard")
    data.load_misc_dashboard()

+    print("Loading DECK.gl demo")


merge resolution mismatch?

still WiP, I'll ping when ready for review...

xrmx · 2017-11-03T15:42:22Z

package.json

@@ -0,0 +1,5 @@
+{


Is this file in the right place?

xrmx · 2017-11-03T15:43:11Z

superset/viz.py

@@ -1731,6 +1689,112 @@ def get_data(self, df):
        }


+class BaseDeckGLViz(BaseViz):
+
+    """Rich maps made with Mapbox"""


c&p description :)

coveralls · 2017-11-03T16:02:25Z

Coverage increased (+0.4%) to 71.648% when pulling afa8443d6992ea822f7b82e0e0ea1c04e34e471c on mistercrunch:deck.gl into 7fd9c82 on apache:master.

coveralls · 2017-11-03T16:12:43Z

Coverage increased (+0.4%) to 71.648% when pulling afa8443d6992ea822f7b82e0e0ea1c04e34e471c on mistercrunch:deck.gl into 7fd9c82 on apache:master.

coveralls · 2017-11-03T16:12:43Z

Coverage increased (+0.4%) to 71.648% when pulling afa8443d6992ea822f7b82e0e0ea1c04e34e471c on mistercrunch:deck.gl into 7fd9c82 on apache:master.

coveralls · 2017-11-03T16:40:20Z

Coverage increased (+0.4%) to 71.648% when pulling 6ead867a92d011feef3b6e04629b0a4ca1d03aeb on mistercrunch:deck.gl into 7fd9c82 on apache:master.

coveralls · 2017-11-05T18:03:54Z

Coverage increased (+0.3%) to 71.566% when pulling 0c49a648d6b4086244d91c4ac200c5d7258fec4b on mistercrunch:deck.gl into 7fd9c82 on apache:master.

coveralls · 2017-11-05T21:11:59Z

Coverage increased (+0.05%) to 71.317% when pulling 6ba6f9a2f2b9a7caa63f201ace49033ef8c5d130 on mistercrunch:deck.gl into 13c17e1 on apache:master.

coveralls · 2017-11-05T21:11:59Z

Coverage decreased (-0.09%) to 71.178% when pulling 6ba6f9a2f2b9a7caa63f201ace49033ef8c5d130 on mistercrunch:deck.gl into 13c17e1 on apache:master.

coveralls · 2017-11-05T21:53:03Z

Coverage increased (+0.05%) to 71.317% when pulling 7618557b432bf7a169dc1952bcccfdcb24821868 on mistercrunch:deck.gl into 13c17e1 on apache:master.

coveralls · 2017-11-05T21:53:03Z

Coverage increased (+0.05%) to 71.317% when pulling 7618557b432bf7a169dc1952bcccfdcb24821868 on mistercrunch:deck.gl into 13c17e1 on apache:master.

coveralls · 2017-11-05T21:54:27Z

Coverage increased (+0.05%) to 71.317% when pulling 7618557b432bf7a169dc1952bcccfdcb24821868 on mistercrunch:deck.gl into 13c17e1 on apache:master.

coveralls · 2017-11-05T21:54:28Z

Coverage increased (+0.05%) to 71.317% when pulling 7618557b432bf7a169dc1952bcccfdcb24821868 on mistercrunch:deck.gl into 13c17e1 on apache:master.

coveralls · 2017-11-06T07:29:04Z

Coverage increased (+0.05%) to 71.317% when pulling c8c6628fce6fd0c0b478af1e8538fcd9e21bb60f on mistercrunch:deck.gl into 13c17e1 on apache:master.

coveralls · 2017-11-06T08:05:31Z

Coverage increased (+0.05%) to 71.317% when pulling 597132fe33216469d8d95529b9108dabb84cd024 on mistercrunch:deck.gl into 13c17e1 on apache:master.

coveralls · 2017-11-07T04:44:51Z

Coverage increased (+0.05%) to 71.512% when pulling b9b79683fa71955c297e9fadc5883c2fdb5a2151 on mistercrunch:deck.gl into ccb87d3 on apache:master.

Adding a new set of geospatial visualizations building on top of the awesome deck.gl library. https://github.com/uber/deck.gl While the end goal it to expose all types of layers and let users bind their data to control most props exposed by the deck.gl API, this PR focusses on a first set of visualizations and props: * ScatterLayer * HexagonLayer * GridLayer * ScreenGridLayer

coveralls · 2017-11-07T05:17:24Z

Coverage increased (+0.05%) to 71.512% when pulling 310496ee0f3e59eda29621829db2451dfb0d4e99 on mistercrunch:deck.gl into ccb87d3 on apache:master.

coveralls · 2017-11-07T05:33:36Z

Coverage increased (+0.05%) to 71.512% when pulling bbe3fca on mistercrunch:deck.gl into ccb87d3 on apache:master.

fabianmenges · 2017-11-07T14:29:42Z

superset/assets/javascripts/explore/components/controls/FixedOrMetricControl.jsx

+  constructor(props) {
+    super(props);
+    this.onChange = this.onChange.bind(this);
+    const type = props.value ? props.value.type : props.default.type || 'fix';


'fix' should probably be a global constant that you reference everywhere. e.g. declare VALUE_TYPE_FIX = 'fix' and use that everywhere. In case you ever want to refactor, this will make it a lot easier.

I think adding parenthesis will make this line easier to read:

const type = props.value ? props.value.type : (props.default.type || 'fix'); vs const type = (props.value ? props.value.type : props.default.type) || 'fix';

I always get confused about the order of operations in these cases, but maybe thats just me.
another option is to pull out if (props.value)

yes like an enum, I'm going with

const controlTypes = { fixed: 'fix', metric: 'metric', };

fabianmenges · 2017-11-07T14:34:53Z

superset/assets/javascripts/explore/components/controls/FixedOrMetricControl.jsx

+    this.state = {
+      type,
+      fixedValue: type === 'fix' ? value : '',
+      metricValue: type === 'metric' ? value : null,


VALUE_TYPE_METRIC instead of metric? Also it looks weird that one of them defaults to '' while the other one defaults to null

one is a string (TextControl) and the other is an unselected SelectControl...

fabianmenges · 2017-11-07T14:41:16Z

superset/assets/javascripts/explore/components/controls/FixedOrMetricControl.jsx

+          <PopoverSection
+            title="Fixed"
+            isSelected={type === 'fix'}
+            onSelect={this.onChange.bind(this, 'fix')}


is there a reason you bind this.onChange here? Afaik calling bind once in the constructor is recommended and more performant (https://reactjs.org/docs/handling-events.html)

We generally recommend binding in the constructor or using the class fields syntax, to avoid this sort of performance problem.

Well here I'm binding with 'fix' as well, I guess I could bind to this on the constructor and re-bind that to 'fix' here.

fabianmenges · 2017-11-07T14:42:51Z

superset/assets/javascripts/explore/components/controls/ViewportControl.jsx

+  onChange: () => {},
+  default: { type: 'fix', value: 5 },
+  value: {
+    longitude: 6.85236157047845,


Could you put in a comment what these coordinate are?

fabianmenges · 2017-11-07T14:47:57Z

superset/assets/javascripts/explore/stores/controls.jsx

@@ -424,6 +432,12 @@ export const controls = {
  },

  groupby: groupByControl,
+  dimension: Object.assign({}, groupByControl, {


Further up you are doing

{ ...this.props.value, [ctrl]: value, }

here you are using Object.assign() instead of

{ ... groupByControl, label: t('Dimension'), description: t('Select a dimension'), multi: false, default: null, }

yeah let's move towards spread operator instead of Object.assign, we should eventually add a linting rule for this

fabianmenges · 2017-11-07T14:49:00Z

superset/assets/javascripts/explore/stores/controls.jsx

+    renderTrigger: true,
+    description: t('Parameters related to the view and perspective on the map'),
+    default: {
+      longitude: 6.85236157047845,


These looks like the same numbers that are hardcoded as defaults, do we need them in both locations?

moving as defaultViewport in utils/geo.js

mistercrunch · 2017-11-08T05:31:30Z

Thanks for the review @fabianmenges ! I addressed all the comments and improved a few other things along the way.

coveralls · 2017-11-08T05:40:34Z

Coverage increased (+0.05%) to 71.492% when pulling 53c3159f770943d8a0d5cc94cbc566d87d571d86 on mistercrunch:deck.gl into 7453131 on apache:master.

coveralls · 2017-11-08T05:46:24Z

Coverage increased (+0.04%) to 71.484% when pulling 53c3159f770943d8a0d5cc94cbc566d87d571d86 on mistercrunch:deck.gl into 7453131 on apache:master.

coveralls · 2017-11-08T05:46:24Z

Coverage increased (+0.04%) to 71.484% when pulling 53c3159f770943d8a0d5cc94cbc566d87d571d86 on mistercrunch:deck.gl into 7453131 on apache:master.

coveralls · 2017-11-08T18:27:52Z

Coverage increased (+0.05%) to 71.494% when pulling f6ef855 on mistercrunch:deck.gl into 39e502f on apache:master.

coveralls · 2017-11-08T18:27:53Z

Coverage increased (+0.05%) to 71.494% when pulling f6ef855 on mistercrunch:deck.gl into 39e502f on apache:master.

coveralls · 2017-11-09T04:47:18Z

Coverage increased (+0.05%) to 71.494% when pulling 1dc080789b0ab8d772b45fbad1edc2ed4e86cbcd on mistercrunch:deck.gl into 39e502f on apache:master.

coveralls · 2017-11-09T04:47:18Z

Coverage increased (+0.05%) to 71.494% when pulling 1dc080789b0ab8d772b45fbad1edc2ed4e86cbcd on mistercrunch:deck.gl into 39e502f on apache:master.

coveralls · 2017-11-09T04:47:18Z

Coverage increased (+0.05%) to 71.494% when pulling 1dc080789b0ab8d772b45fbad1edc2ed4e86cbcd on mistercrunch:deck.gl into 39e502f on apache:master.

coveralls · 2017-11-09T04:47:18Z

Coverage increased (+0.05%) to 71.494% when pulling 1dc080789b0ab8d772b45fbad1edc2ed4e86cbcd on mistercrunch:deck.gl into 39e502f on apache:master.

coveralls · 2017-11-09T05:32:12Z

Coverage increased (+0.05%) to 71.494% when pulling 3202b67 on mistercrunch:deck.gl into cbcc00c on apache:master.

williaster

Super excited for this! Had a few minor comments 🚀

williaster · 2017-11-10T19:35:24Z

superset/assets/javascripts/explore/components/controls/FixedOrMetricControl.jsx

+  value: PropTypes.object,
+  isFloat: PropTypes.bool,
+  datasource: PropTypes.object,
+  default: PropTypes.object,


should this be a shape?

williaster · 2017-11-10T19:37:02Z

superset/assets/javascripts/explore/components/controls/FixedOrMetricControl.jsx

+    });
+  }
+  setType(type) {
+    this.setState({ type }, this.onChange);


setState should be a function now https://reactjs.org/docs/react-component.html#setstate

oh interesting I had not seen that change

After reviewing the updated React docs it doesn't sound like the updater as first arg is a preferred way of calling setState or that passing an object will get deprecated in the future of anything. Any other rational to change thing over? I'd rather not go and change hundreds of setState calls in the code base to add unneeded brackets...

I tried briefly to find a related eslint linting rule that would explain why an updater function is preferred and oculdn't find any...

williaster · 2017-11-10T19:37:17Z

superset/assets/javascripts/explore/components/controls/FixedOrMetricControl.jsx

+    this.setState({ type }, this.onChange);
+  }
+  setFixedValue(fixedValue) {
+    this.setState({ fixedValue }, this.onChange);


ditto functional setState

williaster · 2017-11-10T19:37:48Z

superset/assets/javascripts/explore/components/controls/FixedOrMetricControl.jsx

+    this.setState({ fixedValue }, this.onChange);
+  }
+  setMetric(metricValue) {
+    this.setState({ metricValue }, this.onChange);


ditto functional setState

williaster · 2017-11-10T19:38:24Z

superset/assets/javascripts/explore/components/controls/FixedOrMetricControl.jsx

+          >
+            <TextControl
+              isFloat
+              onChange={this.setFixedValue.bind(this)}


you shouldn't bind in a render function

williaster · 2017-11-10T19:47:18Z

superset/assets/visualizations/deckgl/grid.jsx

+  }));
+
+  const layer = new GridLayer({
+    id: 'screengrid-layer',


should this include the something to be unique?

williaster · 2017-11-10T19:47:44Z

superset/assets/visualizations/deckgl/scatter.jsx

+    data,
+    pickable: true,
+    fp64: true,
+    // onHover: info => console.log('Hovered:', info),


williaster · 2017-11-10T19:48:11Z

superset/assets/visualizations/deckgl/scatter.jsx

+  });
+
+  const layer = new ScatterplotLayer({
+    id: 'scatterplot-layer',


should this include the something to be unique?

williaster · 2017-11-10T19:48:19Z

superset/assets/visualizations/deckgl/hex.jsx

+  }));
+
+  const layer = new HexagonLayer({
+    id: 'screengrid-layer',


should this include the something to be unique?

williaster · 2017-11-10T19:48:42Z

superset/assets/visualizations/deckgl/screengrid.jsx

+  // Passing a layer creator function instead of a layer since the
+  // layer needs to be regenerated at each render
+  const layer = () => new ScreenGridLayer({
+    id: 'screengrid-layer',


should this include the something to be unique?

mistercrunch · 2017-11-12T20:16:47Z

@williaster I addressed your comments except for setState where I think we should stick with the way it's now, and for the spread operator where I think it's out of scope for this PR but I think we should totally remove Object.assign from our code base.

graceguo-supercat · 2017-11-16T08:30:00Z

LGTM

* DECKGL integration Adding a new set of geospatial visualizations building on top of the awesome deck.gl library. https://github.com/uber/deck.gl While the end goal it to expose all types of layers and let users bind their data to control most props exposed by the deck.gl API, this PR focusses on a first set of visualizations and props: * ScatterLayer * HexagonLayer * GridLayer * ScreenGridLayer * Addressing comments * lint * Linting * Addressing chri's comments

xrmx reviewed Nov 3, 2017

View reviewed changes

package.json Outdated

@@ -0,0 +1,5 @@

{

Copy link

Contributor

xrmx Nov 3, 2017 •

edited

Loading

Choose a reason for hiding this comment

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

Is this file in the right place?

xrmx reviewed Nov 3, 2017

View reviewed changes

mistercrunch force-pushed the deck.gl branch from 8e05fa9 to afa8443 Compare November 3, 2017 15:46

mistercrunch force-pushed the deck.gl branch 2 times, most recently from 5c3565f to 6ba6f9a Compare November 5, 2017 20:54

mistercrunch force-pushed the deck.gl branch 2 times, most recently from a595c8c to 7618557 Compare November 5, 2017 21:38

mistercrunch force-pushed the deck.gl branch from c8c6628 to 597132f Compare November 6, 2017 07:49

mistercrunch changed the title ~~[WiP] DECKGL integration~~ [WiP] DECKGL integration - Phase 1 Nov 6, 2017

mistercrunch force-pushed the deck.gl branch from 597132f to b9b7968 Compare November 7, 2017 04:31

mistercrunch force-pushed the deck.gl branch from b9b7968 to 310496e Compare November 7, 2017 05:01

mistercrunch changed the title ~~[WiP] DECKGL integration - Phase 1~~ DECKGL integration - Phase 1 Nov 7, 2017

mistercrunch force-pushed the deck.gl branch from 310496e to bbe3fca Compare November 7, 2017 05:14

fabianmenges reviewed Nov 7, 2017

View reviewed changes

mistercrunch force-pushed the deck.gl branch from a0dd707 to 53c3159 Compare November 8, 2017 05:30

Addressing comments

f6ef855

mistercrunch force-pushed the deck.gl branch from 53c3159 to f6ef855 Compare November 8, 2017 18:11

lint

3202b67

mistercrunch force-pushed the deck.gl branch from 1dc0807 to 3202b67 Compare November 9, 2017 05:16

mistercrunch requested a review from graceguo-supercat November 10, 2017 04:25

williaster reviewed Nov 10, 2017

View reviewed changes

mistercrunch added 2 commits November 10, 2017 21:40

Linting

f8be873

Addressing chri's comments

e920143

graceguo-supercat merged commit 3a8af5d into apache:master Nov 16, 2017

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

DECKGL integration - Phase 1 #3771

DECKGL integration - Phase 1 #3771

Conversation

mistercrunch commented Nov 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xrmx Nov 3, 2017 • edited Loading

Choose a reason for hiding this comment

xrmx Nov 3, 2017 • edited Loading

Choose a reason for hiding this comment

coveralls commented Nov 3, 2017 • edited Loading

coveralls commented Nov 3, 2017

coveralls commented Nov 3, 2017 • edited Loading

coveralls commented Nov 3, 2017 • edited Loading

coveralls commented Nov 5, 2017 • edited Loading

coveralls commented Nov 5, 2017 • edited Loading

coveralls commented Nov 5, 2017 • edited Loading

coveralls commented Nov 5, 2017

coveralls commented Nov 5, 2017 • edited Loading

coveralls commented Nov 5, 2017

coveralls commented Nov 5, 2017 • edited Loading

coveralls commented Nov 6, 2017 • edited Loading

coveralls commented Nov 6, 2017 • edited Loading

coveralls commented Nov 7, 2017 • edited Loading

coveralls commented Nov 7, 2017 • edited Loading

coveralls commented Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistercrunch commented Nov 8, 2017

coveralls commented Nov 8, 2017 • edited Loading

coveralls commented Nov 8, 2017

coveralls commented Nov 8, 2017 • edited Loading

coveralls commented Nov 8, 2017

coveralls commented Nov 8, 2017 • edited Loading

coveralls commented Nov 9, 2017

coveralls commented Nov 9, 2017

coveralls commented Nov 9, 2017

coveralls commented Nov 9, 2017

coveralls commented Nov 9, 2017 • edited Loading

williaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistercrunch commented Nov 12, 2017

graceguo-supercat commented Nov 16, 2017

mistercrunch commented Nov 3, 2017 •

edited

Loading

xrmx Nov 3, 2017 •

edited

Loading

xrmx Nov 3, 2017 •

edited

Loading

coveralls commented Nov 3, 2017 •

edited

Loading

coveralls commented Nov 3, 2017 •

edited

Loading

coveralls commented Nov 3, 2017 •

edited

Loading

coveralls commented Nov 5, 2017 •

edited

Loading

coveralls commented Nov 5, 2017 •

edited

Loading

coveralls commented Nov 5, 2017 •

edited

Loading

coveralls commented Nov 5, 2017 •

edited

Loading

coveralls commented Nov 5, 2017 •

edited

Loading

coveralls commented Nov 6, 2017 •

edited

Loading

coveralls commented Nov 6, 2017 •

edited

Loading

coveralls commented Nov 7, 2017 •

edited

Loading

coveralls commented Nov 7, 2017 •

edited

Loading

coveralls commented Nov 7, 2017 •

edited

Loading

coveralls commented Nov 8, 2017 •

edited

Loading

coveralls commented Nov 8, 2017 •

edited

Loading

coveralls commented Nov 8, 2017 •

edited

Loading

coveralls commented Nov 9, 2017 •

edited

Loading