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

Improvements to the polygon spatial viz #6178

Merged
merged 24 commits into from
Oct 25, 2018
Merged

Conversation

betodealmeida
Copy link
Member

Many improvements to the polygon spatial viz, ported from the (private) ZIP code viz:

Legend for the heatmap

When a metric is selected, we can optionally show a legend mapping values to colors. This uses the same <Legend> component used for showing/toggling categories:

screen shot 2018-10-24 at 10 39 00 am

Bucketing data into groups

By default the legend shows 10 buckets, and the colors are linearly interpolated. It's possible to specify a different number of buckets, eg:

screen shot 2018-10-24 at 10 42 17 am

It's also possible to explicitly define the break points:

screen shot 2018-10-24 at 10 42 50 am

Anything outside the break points is masked out with opacity 0.

Map elevation to a secondary metric

It's now possible to represent elevation using a secondary metric, and users can also specify a multiplier for the elevation:

screen shot 2018-10-24 at 10 44 20 am

Play slider

The polygon spatial viz now can be animated using the play slider. Note that the CSS is broken in master — I'll fix that in a separate PR.

play_slider_polygon_hi

Toggling polygons

It's now possible to toggle polygons when sending events, when the "Multiple filtering" control is enabled. This allows the user to select multiple polygons and filter another chart accordingly:

toggle_geohash

(It also works when "extrude" is false and the viz has no elevation.)

const DOUBLE_CLICK_TRESHOLD = 250; // milliseconds

function getElevation(d, colorScaler) {
/* in deck.gl, if a polygon has opacity zero it will make everything behind
Copy link
Member

Choose a reason for hiding this comment

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

NIT: this looks like a deck.gl bug to me, we may want to comment more precisely: in deck.gl version x.x.x (used in Superset as of 2018-10-12), ...

Copy link
Member Author

Choose a reason for hiding this comment

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

(y)

@@ -2319,15 +2319,19 @@ class DeckPathViz(BaseDeckGLViz):
viz_type = 'deck_path'
verbose_name = _('Deck.gl - Paths')
deck_viz_key = 'path'
is_timeseries = True
Copy link
Member

Choose a reason for hiding this comment

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

I think you'll want the control override time_grain_sqla: timeGrainSqlaAnimationOverrides in visTypes.jsx

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, will add!

superset/viz.py Outdated
@@ -2347,8 +2351,14 @@ def get_properties(self, d):
d[self.deck_viz_key] = path
if line_type != 'geohash':
del d[line_column]
d['metric'] = d.get(self.metric_label)
Copy link
Member

Choose a reason for hiding this comment

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

Does that do anything? I thought the query_obj only took metrics as an arg (?)

Copy link
Member Author

Choose a reason for hiding this comment

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

It's leftover, will remove.

Copy link
Member

@mistercrunch mistercrunch left a comment

Choose a reason for hiding this comment

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

A few minor comments, otherwise looks great!

return fd.break_points.sort((a, b) => parseFloat(a) - parseFloat(b));
}

export function hexToRGB(hex, alpha = 255) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is already a hexToRGB function in colors.js.
Why create a new copy here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it got duplicated because of refactoring. Will remove this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like only deck.gl components are using hexToRGB so you can consider moving it here too. I may try to refactor and deprecate colors.js in the near term.

let scaler;
let maskPoint;
if (breakPoints !== null) {
// bucket colors into discrete colors
Copy link
Contributor

Choose a reason for hiding this comment

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

There is d3.scale.quantize()

Copy link
Member Author

Choose a reason for hiding this comment

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

I looked at it and it didn't work in this case because we want to quantize into buckets of different widths (eg, 0-10 and 10-100).

Copy link
Contributor

Choose a reason for hiding this comment

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

got it.

import Legend from '../../../Legend';
import { getBuckets, getBreakPointColorScaler } from '../../utils';

import * as common from '../common';
Copy link
Contributor

Choose a reason for hiding this comment

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

better import specific functions import { fn } than import * to fully benefit from tree-shaking.

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely, will fix.

import getSequentialSchemeRegistry from '../../modules/colors/SequentialSchemeRegistrySingleton';
import { colorScalerFactory } from '../../modules/colors';

export function getBreakPoints(fd, features) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would strongly recommend listing out the fields you need for this function than the ambiguous fd. This will make it clearer what are needed for this function to work.

export function getBreakPoints({ 
  break_points,
  metric,
  num_buckets,
}, features) {
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool, will do that. Thanks!

return [r, g, b, alpha];
}

export function getBreakPointColorScaler(fd, features) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same issue with fd as above.

Copy link
Member Author

Choose a reason for hiding this comment

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

(y)

@codecov-io
Copy link

Codecov Report

Merging #6178 into master will increase coverage by 0.01%.
The diff coverage is 84%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6178      +/-   ##
==========================================
+ Coverage   76.91%   76.93%   +0.01%     
==========================================
  Files          47       47              
  Lines        9362     9386      +24     
==========================================
+ Hits         7201     7221      +20     
- Misses       2161     2165       +4
Impacted Files Coverage Δ
superset/viz.py 80.98% <84%> (-0.03%) ⬇️
superset/connectors/sqla/models.py 80.94% <0%> (+0.18%) ⬆️

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 6ec9e1a...4776a8e. Read the comment docs.

@mistercrunch
Copy link
Member

LGTM!

@betodealmeida betodealmeida merged commit f1089c4 into apache:master Oct 25, 2018
betodealmeida added a commit to lyft/incubator-superset that referenced this pull request Oct 30, 2018
* WIP

* WIP

* WIP

* WIP

* Fix color bucketing

* Fixed colors

* Fix no num categories selected

* Colors working

* Fix no metric selected

* Visual cues for selection

* Add unit tests

* Remove jest from deps

* Rename category to bucket

* Small fixes

* Fix lint

* Fix unit tests

* Remove duplicate hexToRGB

* Fix import

* Change order of arguments in getBuckets

* Refactor function signature

(cherry picked from commit f1089c4)
betodealmeida added a commit to lyft/incubator-superset that referenced this pull request Oct 30, 2018
* WIP

* WIP

* WIP

* WIP

* Fix color bucketing

* Fixed colors

* Fix no num categories selected

* Colors working

* Fix no metric selected

* Visual cues for selection

* Add unit tests

* Remove jest from deps

* Rename category to bucket

* Small fixes

* Fix lint

* Fix unit tests

* Remove duplicate hexToRGB

* Fix import

* Change order of arguments in getBuckets

* Refactor function signature

(cherry picked from commit f1089c4)
(cherry picked from commit 699813f)
bipinsoniguavus pushed a commit to ThalesGroup/incubator-superset that referenced this pull request Dec 26, 2018
* WIP

* WIP

* WIP

* WIP

* Fix color bucketing

* Fixed colors

* Fix no num categories selected

* Colors working

* Fix no metric selected

* Visual cues for selection

* Add unit tests

* Remove jest from deps

* Rename category to bucket

* Small fixes

* Fix lint

* Fix unit tests

* Remove duplicate hexToRGB

* Fix import

* Change order of arguments in getBuckets

* Refactor function signature
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.34.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.34.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants