Fix multilayer geoviz and color picker error #5767

youngyjd · 2018-08-28T22:53:38Z

Several improvements and bug fixes in this PR.

The multilayer geoviz was unable to be rendered because queries did not filter out null locations by default. Changing if fd.get('filter_nulls') to if fd.get('filter_nulls', True) fixed the issue.
getLayer() function in scatter.jsx is also called by multilayer geoviz directly. After introducing component CategoricalDeckGLContainer, the getLayer() interface got changed so scatterplot failed to be rendered in multilayer geoviz.
color picker does not work with scatter plot and it is always showing black point. This issue is fixed as well.

youngyjd · 2018-08-28T23:03:30Z

codecov-io · 2018-08-28T23:32:40Z

Codecov Report

Merging #5767 into master will increase coverage by <.01%.
The diff coverage is 4.54%.

@@            Coverage Diff             @@
##           master    #5767      +/-   ##
==========================================
+ Coverage   63.81%   63.81%   +<.01%     
==========================================
  Files         364      364              
  Lines       23055    23080      +25     
  Branches     2568     2572       +4     
==========================================
+ Hits        14713    14729      +16     
- Misses       8327     8336       +9     
  Partials       15       15

Impacted Files	Coverage Δ
...et/assets/src/visualizations/deckgl/layers/arc.jsx	`0% <0%> (ø)`	⬆️
...ssets/src/visualizations/deckgl/layers/scatter.jsx	`0% <0%> (ø)`	⬆️
...sualizations/deckgl/CategoricalDeckGLContainer.jsx	`0% <0%> (ø)`	⬆️
...uperset/assets/src/visualizations/deckgl/multi.jsx	`0% <0%> (ø)`	⬆️
superset/viz.py	`81.38% <100%> (+0.09%)`	⬆️
superset/assets/src/visualizations/world_map.js	`0% <0%> (ø)`	⬆️
superset/assets/src/visualizations/rose.js	`0% <0%> (ø)`	⬆️
superset/connectors/sqla/models.py	`80.91% <0%> (+2.29%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2da5db9...ea6ef50. Read the comment docs.

betodealmeida

Looks great, thanks for catching all these little problems! There's just one modification we should do: before we were passing payload, I changed it to pass only data (breaking the generic interface), and now you've changed it to pass both. Let's pass only the payload, and extract data from it.

betodealmeida · 2018-08-29T00:28:09Z

superset/assets/src/visualizations/deckgl/multi.jsx

-    let filters = subslice.form_data.filters.concat(fd.filters);
+    let filters = [];
+    if (subslice.form_data.filters) {
+      filters = filters.concat(subslice.form_data.filters);


Nit, can you use array spread here instead of concat?

filters = [...filters, ...subslice.form_data.filters];

Same below, in the two uses of concat. It's the new preferred way of concatenating arrays.

got you. Thank!

betodealmeida · 2018-08-29T00:30:27Z

superset/assets/src/visualizations/deckgl/multi.jsx

+    }
+    if (fd.filters) {
+      filters = filters.concat(fd.filters);
+    }
    if (fd.extra_filters) {
      filters = filters.concat(fd.extra_filters);
    }


Alternatively, we could do it in a single pass:

const filters = [ ...(subslice.form_data.filters || []) ...(fd.filters || []) ...(fd.extra_filters || []) ];

betodealmeida · 2018-08-29T00:31:16Z

superset/viz.py


        return {
-            'arcs': arcs,
+            'features': d['features'],


+1 I think it's good to standardize on the key value here.

betodealmeida · 2018-08-29T00:32:00Z

superset/assets/src/visualizations/deckgl/layers/scatter.jsx

+    }
+    const c = fd.color_picker || { r: 0, g: 0, b: 0, a: 1 };
+    const color = [c.r, c.g, c.b, c.a * 255];
+    return { ...d, radius, color };


Thanks for fixing this, I think I must've removed it by mistake.

betodealmeida · 2018-08-29T00:33:20Z

superset/assets/src/visualizations/deckgl/layers/arc.jsx

@@ -18,7 +18,8 @@ function getPoints(data) {
  return points;
 }

-function getLayer(fd, data, slice) {
+function getLayer(fd, payload, slice) {


Good catch, I simplified the getLayer interface but forgot that other deck.gl vizs need other attributes in the payload.

betodealmeida · 2018-08-29T00:35:54Z

superset/assets/src/visualizations/deckgl/layers/arc.jsx

  }

  ReactDOM.render(
    <CategoricalDeckGLContainer
      slice={slice}
-      data={payload.data.arcs}
+      data={payload.data.features}


If we're passing payload below then we shouldn't pass data.

betodealmeida · 2018-08-29T00:37:00Z

superset/assets/src/visualizations/deckgl/layers/scatter.jsx

@@ -54,6 +59,7 @@ function deckScatter(slice, payload, setControlValue) {
      setControlValue={setControlValue}
      viewport={viewport}
      getLayer={getLayer}
+      payload={payload}


Remove data above, since we're passing the payload.

Sure! make sense

betodealmeida · 2018-08-29T00:37:30Z

superset/assets/src/visualizations/deckgl/CategoricalDeckGLContainer.jsx

@@ -35,6 +35,7 @@ const propTypes = {
  setControlValue: PropTypes.func.isRequired,
  viewport: PropTypes.object.isRequired,
  getLayer: PropTypes.func.isRequired,
+  payload: PropTypes.object.isRequired,


Let's remove data from the props, since it can fetched from the payload.

youngyjd · 2018-08-29T02:57:41Z

@betodealmeida Comments addressed. PTAL

betodealmeida

@youngyjd looks great! I have just a small comment on the payload inside getLayer, we can change it so data doesn't get copied.

betodealmeida · 2018-08-29T21:04:58Z

superset/assets/src/visualizations/deckgl/CategoricalDeckGLContainer.jsx

@@ -80,7 +80,7 @@ export default class CategoricalDeckGLContainer extends React.PureComponent {
  }
  getLayers(values) {
    const fd = this.props.slice.formData;
-    let data = [...this.props.data];
+    let data = [...this.props.payload.data.features];


Here you're making a copy of this.props.payload.data.features, mutating it, and then assigning it back to payload in line 107. Here you can simply do:

let data = this.props.payload.data.features;

And then as you mutate it, payload.data.features is also mutated. This way you can simply pass the original payload to getLayer.

Did some test and found it does not work. I guess it is because the location that data references got changed so payload.data.features will not get updated.

This is part of the code:

let data = [...this.props.payload.data.features]; // Add colors from categories or fixed color data = this.addColor(data, fd);

If we change it to let data = this.props.payload.data.features; and then data get reassigned by data = this.addColor(data, fd);, it is not referencing to the location of this.props.payload.data.features anymore so this.props.payload.data.features will not be changed.

betodealmeida · 2018-08-29T21:08:20Z

superset/assets/src/visualizations/deckgl/CategoricalDeckGLContainer.jsx

-    return [this.props.getLayer(fd, data, this.props.slice)];
+    const payload = this.props.payload;
+    payload.data.features = data;
+    return [this.props.getLayer(fd, payload, this.props.slice)];


This can simply be written as:

return [this.props.getLayer(fd, this.props.payload, this.props.slice)];

If you follow my comment above. You can also do in the beginning of the function:

const { getLayer, payload, slice } = this.props;

The you can simple refer to getLayer, payload and slice inside the function, instead of this.props.getLayer, this.props.payload and this.props.slice.

A lot of good educations here! Thanks!
The reason why I am making a copy of this.props.payload.data.features, mutating it, and then assigning it back to payload in line 107 is that if I understand correctly, changing in this.props will result in re-render. As we are only passing it to getLayer function and don't need it to be re-rendered, I copy it so props is unaffected and unnecessary re-render can be avoided. This is my understanding but I am not very sure whether it is correct based on your suggestion. Please correct me if I am wrong.

well. I am wrong. Only state change will cause re-render.

betodealmeida · 2018-08-29T21:08:56Z

superset/assets/src/visualizations/deckgl/layers/arc.jsx

@@ -40,17 +41,17 @@ function deckArc(slice, payload, setControlValue) {
  };

  if (fd.autozoom) {
-    viewport = common.fitViewport(viewport, getPoints(payload.data.arcs));
+    viewport = common.fitViewport(viewport, getPoints(payload.data.features));


Nit: you can use data here instead of payload.data.features.

Seems not. data is not defined in function deckArc(slice, payload, setControlValue){} so I feel using data here will not work. It is extracted in function getLayer(fd, payload, slice){} instead.

function getLayer(fd, payload, slice) { const data = payload.data.features; const sc = fd.color_picker; const tc = fd.target_color_picker; return new ArcLayer({ id: `path-layer-${fd.slice_id}`, data, getSourceColor: d => d.sourceColor || d.color || [sc.r, sc.g, sc.b, 255 * sc.a], getTargetColor: d => d.targetColor || d.color || [tc.r, tc.g, tc.b, 255 * tc.a], strokeWidth: (fd.stroke_width) ? fd.stroke_width : 3, ...common.commonLayerProps(fd, slice), }); } function deckArc(slice, payload, setControlValue) { const fd = slice.formData; let viewport = { ...fd.viewport, width: slice.width(), height: slice.height(), }; if (fd.autozoom) { viewport = common.fitViewport(viewport, getPoints(payload.data.features)); } ... }

Oh, you're right. I got confused by the hidden lines, my bad!

youngyjd · 2018-08-29T22:09:25Z

Ready for another review @betodealmeida

betodealmeida

Awesome, thanks! :)

betodealmeida · 2018-08-29T22:36:20Z

superset/assets/src/visualizations/deckgl/layers/arc.jsx

@@ -40,17 +41,17 @@ function deckArc(slice, payload, setControlValue) {
  };

  if (fd.autozoom) {
-    viewport = common.fitViewport(viewport, getPoints(payload.data.arcs));
+    viewport = common.fitViewport(viewport, getPoints(payload.data.features));


Oh, you're right. I got confused by the hidden lines, my bad!

* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit (cherry picked from commit 5437efa)

* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit

* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit (cherry picked from commit 5437efa)

* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit

Fix multilayer goeviz and color picker error

37529d4

fix lint

087ff0e

betodealmeida requested changes Aug 29, 2018

View reviewed changes

remove props.data and improve merging list

fd85b97

youngyjd force-pushed the fix-multilayer-and-color-picker branch from 339a8c9 to fd85b97 Compare August 29, 2018 03:04

betodealmeida requested changes Aug 29, 2018

View reviewed changes

nit

ea6ef50

betodealmeida approved these changes Aug 29, 2018

View reviewed changes

betodealmeida merged commit 5437efa into apache:master Aug 29, 2018

betodealmeida pushed a commit to lyft/incubator-superset that referenced this pull request Oct 12, 2018

Fix multilayer geoviz and color picker error (apache#5767)

1de718f

* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit

wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018

Fix multilayer geoviz and color picker error (apache#5767)

39621de

* Fix multilayer goeviz and color picker error * fix lint * remove props.data and improve merging list * nit

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

Fix multilayer geoviz and color picker error #5767

Fix multilayer geoviz and color picker error #5767

youngyjd commented Aug 28, 2018 •

edited

youngyjd commented Aug 28, 2018

codecov-io commented Aug 28, 2018 •

edited

betodealmeida left a comment

betodealmeida Aug 29, 2018

youngyjd Aug 29, 2018

betodealmeida Aug 29, 2018

youngyjd Aug 29, 2018

betodealmeida Aug 29, 2018

betodealmeida Aug 29, 2018

betodealmeida Aug 29, 2018

betodealmeida Aug 29, 2018

betodealmeida Aug 29, 2018

youngyjd Aug 29, 2018

betodealmeida Aug 29, 2018

youngyjd commented Aug 29, 2018

betodealmeida left a comment

betodealmeida Aug 29, 2018

youngyjd Aug 29, 2018

betodealmeida Aug 29, 2018

youngyjd Aug 29, 2018

youngyjd Aug 29, 2018

betodealmeida Aug 29, 2018

youngyjd Aug 29, 2018 •

edited

betodealmeida Aug 29, 2018

youngyjd commented Aug 29, 2018

betodealmeida left a comment

betodealmeida Aug 29, 2018

Fix multilayer geoviz and color picker error #5767

Fix multilayer geoviz and color picker error #5767

Conversation

youngyjd commented Aug 28, 2018 • edited

youngyjd commented Aug 28, 2018

codecov-io commented Aug 28, 2018 • edited

Codecov Report

betodealmeida 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

youngyjd commented Aug 29, 2018

betodealmeida 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

youngyjd Aug 29, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngyjd commented Aug 29, 2018

betodealmeida left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngyjd commented Aug 28, 2018 •

edited

codecov-io commented Aug 28, 2018 •

edited

youngyjd Aug 29, 2018 •

edited