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

feat: Add deck.gl Heatmap Visualization #23551

Merged
merged 11 commits into from
May 22, 2023

Conversation

Mattc1221
Copy link
Contributor

@Mattc1221 Mattc1221 commented Apr 2, 2023

SUMMARY

Adds the heatmap layer from deck.gl here.

Related SIP: #23553

Rationale:

Superset already has supported functionality for other deck.gl visualization layers (e.g. Arc, Geojson, Grid, Hex, Path, Polygon, Scatter, Screengrid). Since Superset already contains the logic to handle deck.gl layers, it is straightforward and easy to maintain to add new deck.gl layers.

By expanding the range of available deck.gl visualization layers, users will have more options to choose from when creating their visualizations. This will allow them to tailor their visualizations to their specific needs and explore their data in different ways. It will also allow Superset to better compete with other data visualization tools on the market that offer a wider range of visualization options.

Overall, I think adding more deck.gl layers to Superset is a great idea and has the potential to significantly improve the user experience and the insights that users can gain from their data.

Design Decisions

The addition of the Heatmap layer follows the structure of existing deck.gl layers to maintain readability and consistency within the codebase.

Structure:
The directory superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Heatmap/ contains the front end structure for the heat map

  • controlPanel configures the form to grab relevant information specified by the user to create and tune the heatmap.
  • Heatmap.jsx contains functions that define the Heatmap Layer, control the tooltip when hover the chart, and to get relevant data
  • index.js serves as the wrapper for the heatmap component and provides a metadata function to grab information to descrive what this visualization is/does

The Back End code for this feature follows the back end code for other deck.gl visualizations as well, providing functions to structure queried data for the Heatmap component.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

New visualization recording:

Screen.Recording.2023-04-01.at.9.20.50.PM.mov

TESTING INSTRUCTIONS

  1. Start Superset
  2. Click on the Charts tab at the top of the screen
  3. Click the + Chart button at the top right
  4. Load a dataset with geo-positional data (longitude/latitude)
  5. Search the charts for Deck.gl Heatmap or click on the Map chart category
  6. Select deck.gl Heatmap and hit Create New Chart
  7. Update the Longitude & Latitude field to the correct columns
  8. Select a Weight function
  9. Select Create Chart
  10. There should now be a heat map layer present on the screen
    • Note: the underlying map component may not be visible if the MAP_BOX_API_KEY is not set. To set it follow the instructions here
  11. Change the options in the control panel, check that the information tooltips are correct/make sense for each field
  12. Check that when hovering a point on the map, the latitude/longitude shows up correctly

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

@Mattc1221 Mattc1221 changed the title [WIP]feat: Add deck.gl Heatmap Visualization feat: [WIP] Add deck.gl Heatmap Visualization Apr 2, 2023
@villebro
Copy link
Member

villebro commented Apr 4, 2023

This is really exciting work @Mattc1221 ! I'm marking myself as a reviewer and will do my best to review+test this later this week.

@villebro villebro self-requested a review April 4, 2023 06:54
@Mattc1221 Mattc1221 changed the title feat: [WIP] Add deck.gl Heatmap Visualization feat: Add deck.gl Heatmap Visualization Apr 16, 2023
@Mattc1221 Mattc1221 marked this pull request as ready for review April 16, 2023 07:14
@Mattc1221
Copy link
Contributor Author

Hi @villebro I believe this PR should now be ready for review!

However, I noticed the tests have not run and I was wondering if I needed to close the PR and open a new one?

@rusackas
Copy link
Member

Running the tests... thanks for this PR! I hope to review/test as well in a day or three :)

@codecov
Copy link

codecov bot commented Apr 17, 2023

Codecov Report

Merging #23551 (a25f82d) into master (1670275) will decrease coverage by 0.02%.
The diff coverage is 36.11%.

❗ Current head a25f82d differs from pull request most recent head f37ea67. Consider uploading reports for the commit f37ea67 to get more accurate results

@@            Coverage Diff             @@
##           master   #23551      +/-   ##
==========================================
- Coverage   68.23%   68.21%   -0.02%     
==========================================
  Files        1942     1945       +3     
  Lines       75205    75241      +36     
  Branches     8145     8150       +5     
==========================================
+ Hits        51314    51327      +13     
- Misses      21806    21829      +23     
  Partials     2085     2085              
Flag Coverage Δ
hive 53.18% <66.66%> (+<0.01%) ⬆️
javascript 54.50% <25.92%> (-0.02%) ⬇️
mysql 78.93% <66.66%> (-0.01%) ⬇️
postgres 79.01% <66.66%> (-0.01%) ⬇️
presto 53.10% <66.66%> (+<0.01%) ⬆️
python 82.78% <66.66%> (-0.01%) ⬇️
sqlite 77.53% <66.66%> (-0.01%) ⬇️
unit 53.05% <66.66%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...i-core/src/color/colorSchemes/sequential/common.ts 100.00% <ø> (ø)
...plugins/legacy-preset-chart-deckgl/src/factory.tsx 0.00% <ø> (ø)
...preset-chart-deckgl/src/layers/Heatmap/Heatmap.tsx 0.00% <0.00%> (ø)
...d/plugins/legacy-preset-chart-deckgl/src/preset.js 100.00% <ø> (ø)
...cy-preset-chart-deckgl/src/layers/Heatmap/index.ts 66.66% <66.66%> (ø)
superset/viz.py 61.34% <66.66%> (+0.02%) ⬆️
...et-chart-deckgl/src/layers/Heatmap/controlPanel.ts 83.33% <83.33%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Mattc1221
Copy link
Contributor Author

@rusackas Thanks for re-running the tests :)

The failing tests should now be fixed!

@@ -0,0 +1,82 @@
/**
Copy link
Member

@kgabryje kgabryje Apr 19, 2023

Choose a reason for hiding this comment

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

Could you please refactor this file to typescript? I know that the rest of deckgl plugins are written in javascript, but our policy is to create the new files in typescript and SOME day refactor the old js files to ts too 🙂 (correct me if I'm wrong @rusackas)

@@ -0,0 +1,45 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

Same as above - js -> ts 🙏

@kgabryje
Copy link
Member

Awesome first contribution, congrats! I left a comment about refactoring js to ts, but otherwise the code looks great 🎉

@Mattc1221
Copy link
Contributor Author

Thanks for the review @kgabryje 😀I'll update those JavaScript files to typescript sometime tomorrow evening!

@Mattc1221 Mattc1221 requested a review from kgabryje April 23, 2023 18:35
@Mattc1221
Copy link
Contributor Author

@kgabryje I really appreciate your first review! The changes requests should now be addressed and I would love to get another round of review!

@kgabryje
Copy link
Member

kgabryje commented May 4, 2023

@Mattc1221 Sorry for the long delay and thank you for implementing the changes 🙂 Will merge as soon as the CI passes

@kgabryje
Copy link
Member

kgabryje commented May 4, 2023

@Mattc1221 looks like there are some linting issues.
Also, to fix the failed Python integration test, please rebase on the latest master

@Mattc1221
Copy link
Contributor Author

@kgabryje Thanks again for the review, and I apologize for the delayed response! I've rebased this branch to include the latest changes on the master branch and fixed the linting errors for the failing file plugins/legacy-preset-chart-deckgl/types/external.d.ts!

@Mattc1221
Copy link
Contributor Author

Hey @kgabryje! could we run the CI tests again? With the latest changes, the tests pass locally and also on my GitHub actions page!

Screenshot 2023-05-20 at 2 18 16 PM

@villebro
Copy link
Member

@Mattc1221 I started CI and will review today 👍

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM, really excited to see geospatial heatmaps arrive in Superset 🎉

@villebro villebro merged commit fc8c537 into apache:master May 22, 2023
29 checks passed
@v1bg
Copy link

v1bg commented May 22, 2023

Hi! This is really cool. Just deployed out and tested and everything looks good with the new heatmap chart type, but just FYI when you try to add the heatmap to the "deck.gl Multiple Layers" chart type it doesn't appear to be working. Wanted to mention this to see if it's a known issue.

Thanks again!

@Mattc1221
Copy link
Contributor Author

Big thanks to @kgabryje @villebro and @v1bg for reviewing this PR!

Hi! This is really cool. Just deployed out and tested and everything looks good with the new heatmap chart type, but just FYI when you try to add the heatmap to the "deck.gl Multiple Layers" chart type it doesn't appear to be working. Wanted to mention this to see if it's a known issue.

This may have been an oversight and may be a missing import in the Multiple Layers chart type. I'll look into this further later this week and open a corresponding issue!

@v1bg
Copy link

v1bg commented May 23, 2023

Awesome, appreciate it @Mattc1221! Great work here 🙏🏾

@Mattc1221 Mattc1221 mentioned this pull request Jun 10, 2023
9 tasks
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 labels Mar 13, 2024
@salvatorecolori
Copy link

salvatorecolori commented May 24, 2024

Hi, does this plugin/chart have extraProps enabled for javascript tooltips generator? When I try to use js code to generate tooltpis, I receive a javascript Uncaught TypeError: Cannot read properties of undefined (reading 'extraProps')".
Please check (url)#25871 (comment) too

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 size/L 🚢 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants