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

Added DeckGL.Polygon Layer w/ JS controls #4227

Merged
merged 15 commits into from
Jan 18, 2018
Merged

Added DeckGL.Polygon Layer w/ JS controls #4227

merged 15 commits into from
Jan 18, 2018

Conversation

hughhhh
Copy link
Member

@hughhhh hughhhh commented Jan 17, 2018

  • Added new layer with advanced JS controls

poly_shot

Closes #4216

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.

You need a png thumbnail here as well here

type: 'SelectControl',
label: t('Polygon Column'),
validators: [v.nonEmpty],
description: t('Select the polygon column'),
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we're assuming JSON formatted string here based on the code bellow. We should be clear about that here. The column the user points to should contain JSON array(N) of array(2).

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we ever use polyline for polygons, seems like it should work for both. We may want to support both as we do for deck_path (optional). That's only immediately useful if we have that input format internally.

label: t('Query'),
expanded: true,
controlSetRows: [
['polygon', 'row_limit'],
Copy link
Member

Choose a reason for hiding this comment

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

We may want to support the reverse control here. Seems like so far I've seen both lat/long long/lat in different places.

superset/viz.py Outdated
return d[self.form_data.get('line_column')]

def query_obj(self):
d = super(DeckPolygon, self).query_obj()
Copy link
Member Author

Choose a reason for hiding this comment

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

@mistercrunch not sure if you wanted me to move this functionality into the baseClass? I think this is fine since we are still leveraging the line_column controls. Let me know what you think.

Copy link
Member

@mistercrunch mistercrunch Jan 18, 2018

Choose a reason for hiding this comment

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

In theory we could have some intermediate (or mixin class) in between the base class that would be common to deck_path and deck_polygon, but in general the python philosophy is to keep inheritance schemes simple when possible. I'm fine leaving it as is as it's little code duplication as it is.

Copy link
Member

Choose a reason for hiding this comment

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

Now that I think about it, could we just have DeckPolygon derive DeckPath? Aren't they pretty much identical?

@mistercrunch
Copy link
Member

Man that screenshot looks amazing.

superset/viz.py Outdated
'polyline': polyline.decode,
}

def get_position(self, d):
Copy link
Member

Choose a reason for hiding this comment

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

My recent PR got rid of get_position, you may need to rebase and remove this method.

superset/viz.py Outdated

def get_properties(self, d):
fd = self.form_data
deser = self.deser_map[fd.get('line_type')]
Copy link
Member

Choose a reason for hiding this comment

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

We could save one hash lookup per row (get_properties gets called for every row in the df) by setting self.deser = self.deser_map[fd.get('line_type')] in __init__. (I missed out on this simple optimization in DeckPath...)

superset/viz.py Outdated
if fd.get('reverse_long_lat'):
polygon = (polygon[1], polygon[0])
return {
'polygon': polygon,
Copy link
Member

Choose a reason for hiding this comment

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

Since this is pretty much the only difference between DeckPath and DeckPolyline, I'd suggest have DeckPolyline derive DeckPath, and renaming this key to something generic like points. If we wanted to have a different key on each context, you could do it through a class (static) attribute that would be different for each class.

@mistercrunch
Copy link
Member

It took me a moment to realize how similar path and polygon are (a polygon is pretty much just a closed path). Now that we know they are pretty much identical from a backend standpoint, we shouldn't have to duplicate any code as one can derive the other.

@williaster
Copy link
Contributor

@hughhhh can you give a screenshot of the new UI controls? 😍

@mistercrunch
Copy link
Member

@williaster
screen shot 2018-01-18 at 12 49 15 pm

@mistercrunch mistercrunch merged commit 5079b2a into apache:master Jan 18, 2018
hughhhh added a commit to lyft/incubator-superset that referenced this pull request Jan 18, 2018
* Working polygon layer for deckGL

* add js controls

* add thumbnail

* better description

* refactor to leverage line_column controls

* templates: open code and documentation on a new tab (apache#4217)

As they are external resources.

* Fix tutorial doesn't match the current interface apache#4138 (apache#4215)

* [bugfix] markup and iframe viz raise 'Empty query' (apache#4225)

closes apache#4222

Related to: apache#4016

* [bugfix] time_pivot entry got missing in merge conflict (apache#4221)

PR here apache#3518 missed a
line of code while merging conflicts with time_pivot viz

* Improve deck.gl GeoJSON visualization (apache#4220)

* Improve geoJSON

* Addressing comments

* lint

* refactor to leverage line_column controls

* refactor to use DeckPathViz

* oops
@williaster
Copy link
Contributor

hmm, does this not directly tie database entries (user-entered JS) to the current ES version/version of babel we are using, as well as the implementation of a given component (e.g., now we are tied to the contract of d.object d.object.props etc)?

that seems like a nightmare for backwards compatibility?

@mistercrunch
Copy link
Member

I'm assuming JS won't evolve in a way that isn't backward compatible here. The supported language is what the "vm" module specs out. That code is sandboxely evaled at runtime, it's not babelized. Here's the construct we're tied to: https://nodejs.org/api/vm.html#vm_script_runinnewcontext_sandbox_options

The contract should not change here, at least not in incremental release of deck.gl and/or Superset.

I've been meaning to add an environment feature flag for this if you think it's necessary to allow folks to turn it off, and possibly a RBAC perm so that only some users are allowed to author JS. Let me know what you think.

Note that we haven't pushed a release since this has been merged and wanted to bring this up at our next committer meeting.

@mistercrunch
Copy link
Member

Quick about the fact that this JS construct is extremely powerful in that it allows us to tackle the very long tail of visualization properties and attributes that [power] users may want to set or bind to the data in specific ways. It would takes dozens of new controls to expose a fraction of what is possible through JS.

@williaster
Copy link
Contributor

thanks for all the additional context etc., I just didn't want to merge something that might have big API/migration implications lightly. I agree that this provides a much more flexible system for data munging and visual attribute mapping. the sandboxed vm seems reasonable.

let's discuss further at the next committer meeting 👍

michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018
* Working polygon layer for deckGL

* add js controls

* add thumbnail

* better description

* refactor to leverage line_column controls

* templates: open code and documentation on a new tab (apache#4217)

As they are external resources.

* Fix tutorial doesn't match the current interface apache#4138 (apache#4215)

* [bugfix] markup and iframe viz raise 'Empty query' (apache#4225)

closes apache#4222

Related to: apache#4016

* [bugfix] time_pivot entry got missing in merge conflict (apache#4221)

PR here apache#3518 missed a
line of code while merging conflicts with time_pivot viz

* Improve deck.gl GeoJSON visualization (apache#4220)

* Improve geoJSON

* Addressing comments

* lint

* refactor to leverage line_column controls

* refactor to use DeckPathViz

* oops
@alldoami
Copy link

@mistercrunch how do I change one of the props in the functions of DeckGL? For example, I want to change the color prop in grid.jsx within Superset. Is that possible?

wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
* Working polygon layer for deckGL

* add js controls

* add thumbnail

* better description

* refactor to leverage line_column controls

* templates: open code and documentation on a new tab (apache#4217)

As they are external resources.

* Fix tutorial doesn't match the current interface apache#4138 (apache#4215)

* [bugfix] markup and iframe viz raise 'Empty query' (apache#4225)

closes apache#4222

Related to: apache#4016

* [bugfix] time_pivot entry got missing in merge conflict (apache#4221)

PR here apache#3518 missed a
line of code while merging conflicts with time_pivot viz

* Improve deck.gl GeoJSON visualization (apache#4220)

* Improve geoJSON

* Addressing comments

* lint

* refactor to leverage line_column controls

* refactor to use DeckPathViz

* oops
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.23.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.23.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for deck.gl "polygon" layer
6 participants