-
Notifications
You must be signed in to change notification settings - Fork 14k
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
[Geo] Added DeckGL Arc Layer and Refactor on BaseDeckGL class #4134
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall but pointed out a few improvements
filled: true, | ||
stroked: false, | ||
extruded: true, | ||
pointRadiusScale: fd.point_radius_scale, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these part of the ArcLayer api? Looks like the docs only reference strokeWdith
, here's a link to the component:
https://github.com/uber/deck.gl/blob/2f59d1a4145ba5fcc6ea5a8380813e94a083d7c5/src/core-layers/arc-layer/arc-layer.js#L32
It'd be nice to allow users to control strokeWidth too
superset/viz.py
Outdated
data.append({ | ||
'sourcePosition': pos[0], | ||
'targetPosition': pos[1], | ||
'color': [255, 0, 0], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
always red? Let's use the color picker here and perhaps do this on the client side so it can take effect without re-running a query.
superset/viz.py
Outdated
gb += [spatial.get('lonlatCol')] | ||
elif spatial.get('type') == 'geohash': | ||
gb += [spatial.get('geohashCol')] | ||
for spatial_key in ['spatial', 'start_spatial', 'end_spatial']: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[optional] could define a process_spatial_control
method in the base deckgl class, and use it in the subclasses that need it.
@mistercrunch I added the new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great if we got rid of the previous spatial
control logic as you refactor this.
One way to model it would be to have the derivatives of BaseDeckGL class specify something like spatial_control_keys = ['spatial']
or spatial_control_keys = ['start_spatial', 'end_spatial']
as a static class attribute. Then the BaseDeckGL.query_obj
method can prepare the query object (prepare the groupby
key in the dict) and BaseDeckGL.get_data
can prepare the dataframe (unroll Series in the dataframe). The derived class would just receive the objects already prepared for used.
superset/viz.py
Outdated
fd = self.form_data | ||
spatial = fd.get(spatial_key) | ||
if spatial is None: | ||
raise Exception(_('Bad spatial key')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: ValueError
superset/viz.py
Outdated
spatial = fd.get(spatial_key) | ||
|
||
if spatial is None: | ||
raise Exception(_('Bad spatial key')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: ValueError
superset/viz.py
Outdated
del df[spatial.get('lonlatCol')] | ||
elif spatial.get('type') == 'geohash': | ||
latlong = df[spatial.get('geohashCol')].map(geohash.decode) | ||
df['lat'] = latlong.apply(lambda x: x[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idea: it may be less complex overall to simply store the lat/long as a tuple in a single column
This reverts commit 8d01b2a.
superset/viz.py
Outdated
|
||
if spatial.get('type') == 'latlong': | ||
df = df.rename(columns={ | ||
spatial.get('lonCol'): 'lon', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't names conflicting here when there are multiple spatial controls? For that reason I think it'd be ideal to use the key as the column name in the dataframe, and store a tuple with lat & long as the value.
…#4134) * Added DeckGL.arc layer * added color controls * added stroke_width control * added process spatial key methods * change exception to ValueError * put location into tuple * reference global spatial keys array * linting * refactor on process_spatial_data_obj * rm whitespace * refactor arc.get_data * Revert "refactor arc.get_data" This reverts commit 8d01b2a. * add spatial controls array * refactor on spatial keys again :) * return altered df * Working refactor with deckGL Arcs * working arcs refactor :) * refactored all other deckGL viz types
…#4134) * Added DeckGL.arc layer * added color controls * added stroke_width control * added process spatial key methods * change exception to ValueError * put location into tuple * reference global spatial keys array * linting * refactor on process_spatial_data_obj * rm whitespace * refactor arc.get_data * Revert "refactor arc.get_data" This reverts commit 8d01b2a. * add spatial controls array * refactor on spatial keys again :) * return altered df * Working refactor with deckGL Arcs * working arcs refactor :) * refactored all other deckGL viz types
DeckGL.ArcLayer()
layer to be used in SupersetBaseDeckGL
lonlat
Closes #4087
@mistercrunch