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

[POC] add echarts scatter #8229

Closed
wants to merge 1 commit into from
Closed

[POC] add echarts scatter #8229

wants to merge 1 commit into from

Conversation

mistercrunch
Copy link
Member

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

REVIEWERS

@@ -0,0 +1,69 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be part of the plugin.

@@ -30,6 +30,7 @@ import BigNumber from './BigNumber';
import BigNumberTotal from './BigNumberTotal';
import BoxPlot from './BoxPlot';
import Bubble from './Bubble';
import EchartsScatter from './EchartsScatter';
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is outdated. This is not necessary anymore.

@@ -2765,6 +2765,15 @@ def get_data(self, df):
return self.nest_values(levels)


class EchartsScatter(BubbleViz):
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you use the new api instead?

Copy link
Member

Choose a reason for hiding this comment

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

@kristw is there already a working plan for migrating some of the more complex legacy viz's to the new api, namely the ones that rely on Pandas functionality? Is the plan to do that logic in JS, och making the heavy lifting in the backend? I anticipate quite a bit of work there, and can lend a hand if necessary, especially if the new API needs new functionality..

Copy link
Contributor

@kristw kristw Sep 16, 2019

Choose a reason for hiding this comment

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

We had light discussions about listing postprocessing operations as a new field in the api/v1/query parameters to call those pandas functionality in addition to the simple table query that it already support, but none had the cycle to draft the API spec nor work on it yet. If you would like to step in, that could be helpful.

I was thinking something along the line of

const queryFormData = {
  group_by,
  adhoc_filters, 
  ...etc.
  // Add a new field that takes an array of post-processing operations
  // which we could enforce the schema in typescript 
  // then you need to modify /api/v1/query in python to handle it.
  postProcessing = [ { operation: '', options: {...} } ];
}

See superset-ui/query for current QueryFormData schema.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds reasonable. In all, I think this will be a big feature to implement, but can probably be dissected into smaller parts. I would probably start with implementing the pivot operation, as it is used quite frequently in viz's and should be easy to start with. I'm fairly overloaded with other stuff right now, but should be able to look into this in a few weeks time if nobody wants to take the lead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Has anyone looked at this? Not sure if it is suitable here https://github.com/nickslevine/zebras

Copy link
Member

Choose a reason for hiding this comment

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

It also doesn't have to be either done in the frontend or backend; can be both. Some postprocessing can be very expensive to move to the front, especially percentiles/quantiles and the like that require having a full distribution, so those would be preferable to perform in the backend, while others might be well suited for the frontend. So I think being able to offer at least some postprocessing options in the backend would be nice, leaving it up to the viz plugin developer to decide where to perform the calculations.

@mistercrunch
Copy link
Member Author

Thanks for the early attention to this draft, this is my very first attempt at using the visualization plugins and was excited to get something half working fairly quickly.

Planning on using the latest api/v1 + controlPanel defined in-plugin, and getting as much as possible of the Bubble chart working.

@stale
Copy link

stale bot commented Nov 18, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

@stale stale bot added the inactive Inactive for >= 30 days label Nov 18, 2019
@fengye712
Copy link

anybody who has succeeded add echarts in superset ? please shared a sample for us , thanks . I have searched in network but most information of it is about 0.28 . Hardly any informations about 0.34

@stale stale bot removed the inactive Inactive for >= 30 days label Nov 22, 2019
@mistercrunch
Copy link
Member Author

@fengye712 this proof of concept pretty much does that, there might be a bit more work to do, but I had things running here. FYI this works along with: apache-superset/superset-ui-plugins#202

@hefeng11725
Copy link

hefeng11725 commented Jan 8, 2020

i have meet some problem when follow the step
like this:
image
Is the EchartsScatter.js file something wrong?

@stale
Copy link

stale bot commented Mar 8, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

@stale stale bot added the inactive Inactive for >= 30 days label Mar 8, 2020
@mistercrunch mistercrunch added the .pinned Draws attention label Mar 8, 2020
@stale stale bot removed the inactive Inactive for >= 30 days label Mar 8, 2020
@mistercrunch mistercrunch deleted the echarts_poc branch March 25, 2024 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.pinned Draws attention size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants