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 Contour Layer #24154

Merged
merged 41 commits into from
Oct 10, 2023
Merged

Conversation

Mattc1221
Copy link
Contributor

@Mattc1221 Mattc1221 commented May 21, 2023

SUMMARY

Adds the Contour Layer from deck.gl here

Related SIP: #23553

Design Decisions:

The addition of the Contour 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/Contour/ contains the Front end structure for the contour layer visualization.

  • controlPanel configures the form to grab relevant information specified by the user to create and tune the contour layer.
  • Contour.tsx contains the functions that define the Contour Layer and get relevant data.
  • index.ts serves as the wrapper for the contour component and provides a metadata function to grab information to describe what this visualization is/does.

The contour layer requires a unique input of "contours". Contours are defined as either Isolines or Isobands (more information here).

  • Isolines require three fields, threshold, color, strokeWidth. threshold defines a value to separate aggregated values on the Map. strokeWidth defines the width of the line in pixels that separates these values. color is just the color of that line.
  • Isobands are similar to Isolines, except the threshold takes 2 values to create polygons on the map instead of lines.

To account for this unique structure, I added a new control panel input component:
Screenshot 2023-05-20 at 11 38 33 PM
The added files for this input component are located under superset-frontend/src/explore/components/controls/ContourControl/:

  • types.ts contains all the types needed for this new component.
  • index.tsx controls the stateful variables that control adding/editing/removing contours.
  • ContourOption.tsx displays added contours and an appropriate tooltip.
  • ContourPopoverControl.tsx defines a popup with appropriate inputs for creating/editing an isoline and isoband.
  • ContourPopoverTrigger.tsx wraps the ContourPopoverControl and controls when it is displayed.

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-05-20.at.11.50.42.PM.mov
Changing order of contours
Screen.Recording.2023-05-21.at.12.11.38.AM.mov

TESTING INSTRUCTIONS

Set up

  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 Contour or click on the Map chart category
  6. Select deck.gl Contour 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 contour 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
    • Note: It may look like the contour layer is not working if the Cell Size parameter is not large enough. For example, using the flights dataset, you should increase Cell Size to 50000.

Check that the following cause an immediate update

  • Check that increasing/decreasing the Cell Size control
  • Try adding a new isoline by clicking Click to add new contour
  • Try adding a new isoband by clicking Click to add new contour
  • Try clicking an existing contour option and ensure that a popover appears with the appropriate fields pre-filled
  • Delet an existing contour option

Other Checks:

  • Check that hovering a contour option and check that the tooltip shows appropriate information
  • Check that in the contour popover, the submit button is disabled when:
    • Not all fields are filled
    • All fields are filled, but a non-integer is entered in any of the textfields
  • Check all descriptions make sense
  • Check that changing the order of contours (by drag and drop) updates their z-index.
    • If there are overlapping contours, contours at the top of the list may be hidden by contours beneath them.
    • Note: z-fighting may occur depending on how many contours are defined/how many are displayed. However this is an issue with the library itself.

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

@Mattc1221 Mattc1221 marked this pull request as ready for review May 21, 2023 05:20
@codecov
Copy link

codecov bot commented May 21, 2023

Codecov Report

Merging #24154 (231620b) into master (5bdb774) will decrease coverage by 0.13%.
The diff coverage is 14.36%.

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

@@            Coverage Diff             @@
##           master   #24154      +/-   ##
==========================================
- Coverage   69.08%   68.95%   -0.13%     
==========================================
  Files        1906     1913       +7     
  Lines       74159    74333     +174     
  Branches     8161     8196      +35     
==========================================
+ Hits        51235    51260      +25     
- Misses      20805    20954     +149     
  Partials     2119     2119              
Flag Coverage Δ
hive 53.94% <66.66%> (+<0.01%) ⬆️
javascript 55.60% <11.51%> (-0.19%) ⬇️
mysql 79.39% <66.66%> (-0.01%) ⬇️
postgres 79.48% <66.66%> (-0.01%) ⬇️
presto 53.83% <66.66%> (+<0.01%) ⬆️
python 83.47% <66.66%> (-0.01%) ⬇️
sqlite 78.04% <66.66%> (-0.01%) ⬇️
unit 54.69% <66.66%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
...preset-chart-deckgl/src/layers/Contour/Contour.tsx 0.00% <0.00%> (ø)
...d/plugins/legacy-preset-chart-deckgl/src/preset.js 100.00% <ø> (ø)
.../controls/DndColumnSelectControl/OptionWrapper.tsx 62.79% <0.00%> (-3.07%) ⬇️
.../explore/components/controls/TextControl/index.tsx 86.66% <ø> (ø)
...-frontend/src/explore/components/controls/index.js 100.00% <ø> (ø)
...plore/components/controls/ContourControl/index.tsx 8.57% <8.57%> (ø)
.../controls/ContourControl/ContourPopoverControl.tsx 10.38% <10.38%> (ø)
.../controls/ContourControl/ContourPopoverTrigger.tsx 14.28% <14.28%> (ø)
...mponents/controls/ContourControl/ContourOption.tsx 21.05% <21.05%> (ø)
...et-chart-deckgl/src/layers/Contour/controlPanel.ts 50.00% <50.00%> (ø)
... and 2 more

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

@Mattc1221
Copy link
Contributor Author

@kgabryje, @rusackas and @villebro

Since you reviewed #23551 I was wondering if you would take a look at this PR as well?

@Mattc1221
Copy link
Contributor Author

Mattc1221 commented Aug 25, 2023

Thanks @kgabryje for the detailed review, I believe I've addressed all of your comments!

  • I can change the contour type from isoline to isoband, but I can't do the opposite - when I edit an isoband, change the tab to isoline and type value, it only edits the lower threshold of isoband. I think user could expect that the contour becomes an isoline.
  • The popover should close after clicking save
  • Maybe we should have some kind of validation to ensure that upper threshold is larger than lower threshold?

You should now be able to change isobands to isolines, the Save button closes the form, I've added some validation messages and disabled saving when the lower threshold is greater than or equal to the upper threshold. The tooltip should now appear with the lat/long coordinates as well as the threshold for whatever contour the mouse is hovering.

Screen.Recording.2023-08-25.at.2.05.36.AM.mov

Note: I will re-request review once all tests are passing!

@kgabryje
Copy link
Member

Thanks for updates @Mattc1221! 1 last thing - for some reason, the inputs unfocus after typing 1 character, which makes typing more than 1 digit difficult. I didn't notice that happening before the latest changes

Screen.Recording.2023-08-28.at.13.18.56.mov

@Mattc1221
Copy link
Contributor Author

Mattc1221 commented Sep 29, 2023

@kgabryje I'm sorry about the delay!

The last comment on the unfocusing issues should now be fixed. I had pulled out the tabs sections into functions within the component. This was causing them to get re-rendered on every change and thus lose focus. I've put them inside of the return statement of the surrounding component and that seems to have fixed the issue

Screen.Recording.2023-09-28.at.10.44.51.PM.mov

@Mattc1221
Copy link
Contributor Author

@villebro @kgabryje @rusackas @justinpark @michael-s-molina I was wondering if one of you could take a final pass through this PR. All of the previous comments have been addressed and the PR should be ready to go.

Copy link
Member

@kgabryje kgabryje left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you @Mattc1221 for the hard work... and patience 😄

@kgabryje kgabryje merged commit 512fb9a into apache:master Oct 10, 2023
29 checks passed
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024
sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 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 size/XXL 🚢 3.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants