feat: add ECharts BoxPlot chart#11199
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11199 +/- ##
==========================================
+ Coverage 65.61% 66.61% +0.99%
==========================================
Files 873 874 +1
Lines 42315 42319 +4
Branches 3971 3972 +1
==========================================
+ Hits 27765 28190 +425
+ Misses 14433 14031 -402
+ Partials 117 98 -19
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
4a57322 to
fa55a69
Compare
602bdf3 to
23a2975
Compare
0644d67 to
3d5d5e2
Compare
There was a problem hiding this comment.
For some reason this test started flaking out on the legacy plugins, despite those being unchanged. Although I xited this test, I updated it in a way which IMO should work.
fde5283 to
a8e4a32
Compare
bkyryliuk
left a comment
There was a problem hiding this comment.
overall everything looks good! let me test it on staging once conflicts are resolved
tests/pandas_postprocessing_tests.py
Outdated
There was a problem hiding this comment.
Good catch, let me take a look
There was a problem hiding this comment.
I meant prior to your PR, will test now if that is still an issue
There was a problem hiding this comment.
I was able to reproduce. Interesting edge case with so few observations, I'll replicate with some other commonly used stats package to see what it would give for a similar case.
a8e4a32 to
2f1c2de
Compare
f959a51 to
12f6f6d
Compare
I'll make a follow-up PR to use the same wording as in the previous version. |
|
For reference Bogdan's sample data in Superset and results for same dataset from two different online boxplot calculators: |
1f92ed7 to
6761bf8
Compare
b3d02a6 to
b011cd9
Compare
b011cd9 to
1bc8118
Compare
| cy.verifySliceSuccess({ waitAlias: '@postJson' }); | ||
| }); | ||
|
|
||
| xit('Should not load mathjs when not needed', () => { |
There was a problem hiding this comment.
@ktmud (pinging as I noticed you had introduced this test recently) I had to disable this test, as apparently the function annotation layer that was added to the ECharts Timeseries viz loads mathjs despite not being used here. The relevant code is here (is called by transformProps.ts): https://github.com/apache-superset/superset-ui/blob/master/plugins/plugin-chart-echarts/src/utils/annotation.ts . Is there something that can be done differently to avoid having mathjs load here?
There was a problem hiding this comment.
Ok, good to know, I'll do that 👍
There was a problem hiding this comment.
@ktmud I've now upgraded all mathjs deps to 8.0.1, and still failing the test. I propose disabling it for now so we can merge this.
There was a problem hiding this comment.
The test is failing because of this import chain: @superset-ui/plugin-chart-echarts -> Timeseries -> transformProps -> utils/annotation -> mathjs. Unlike loadChart, transformProps is not loaded asynchronously, so Webpack could not defer the loading of mathjs, which means it will be loaded for all first-visits on almost all pages, undoing part of the optimization we did in #10837 .
The test is there to make sure this optimization still works. It didn't break because it was flaky. It caught the breaking changes it is supposed to catch.
It's OK to merge this PR as is for now, but I'd recommend finding ways to move the mathjs logic to the chart renderer so that we could continue benefiting from code split and asynchronous loading.
In general I think transformProps should be as lightweight as possible and preferably only return scalars. It should only be about applying defaults and consolidating parameter names/shapes. Most complex logics should be dealt with either by the chart renderer itself or at another data-processing layer on top of the chart. This way it's easier to reuse your chart at other places outside of Superset charting plugins.
There was a problem hiding this comment.
Thanks for the context. I'll move the mathjs (and any other similar) rendering logic out of transformProps and re-enable the test in a subsequent bump.
ec85f7e to
27e58c9
Compare
27e58c9 to
83c5d0f
Compare
6ee05f3 to
c39f18a
Compare
* feat: add ECharts BoxPlot chart * lint * fix cypress tests * lint * remove viz.py shim * bump plugin package * skip non-legacy plugin cypress dashboard tests * fix cypress tests * disable cypress tests for non-leagcy charts * fix bad rebase * use midpoint interpolation for quartile calculation * bump packages and add support for no groupby * whitespace * whitespace * linting * fix tests * xit mathjs load test * bump mathjs to 8.0.1 * disable cypress filter test for v1 charts
|
@villebro i think it make sense to expand box plot to include candlesticks chart and whisker chart two variations since they share the same set of control, just like you did for time series. what do you think for post 1.0 hardening? |
|
@junlincc hmm, not a bad idea, as candlesticks just use pick slightly different data points from within the group (when we do that it might make sense to rename the |









SUMMARY
Replace the BoxPlot chart with an ECharts implementation (The bulk of work has been done in apache-superset/superset-ui#801 ). Features of the new viz plugin:
boxplotfunction in R. See?boxplot.stats:Also introduces a new dedicated post processing operation for boxplot to make it easier for viz plugin developers to tap into this functionality.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
World's Bank Data example dashboard after:

World's Bank Data example dashboard before:

Control panels:


TEST PLAN
CI + new tests
ADDITIONAL INFORMATION