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

fix(plugin-chart-echarts): support truncated numeric x-axis #26215

Merged
merged 3 commits into from
Dec 8, 2023

Conversation

villebro
Copy link
Member

@villebro villebro commented Dec 8, 2023

SUMMARY

A recent PR that added support for numerical x-axes caused unintuitive results when the x-axis contained a highly constrained interval, e.g. the years 1980-2017. In this case, the x-axis was bounded between [0, 7000], causing a highly spiked appearance. This is in contrast with other visualization tools, that default automatically truncating the x-axis.

This PR adds two new x-axis truncation controls, similar to the existing ones for the y-axis. With this change, the x-axis will be truncated by default. If the lower and/or upper bounds are left undefined, they will default to the min and max values from the dataset (calculated automatically by ECharts). This ensures that the data is bounded correctly.

AFTER

Now the x-axis is automatically truncated (see the new "TRUNCATE X AXIS" control, which defaults to being checked):
image

By defining the lower and/or upper bound, the x-axis is updated accordingly:
image

BEFORE

When disabling x-axis truncation, the current "dirac delta"-like behavior is reproduced on the Video Game example chart:
image

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

@villebro
Copy link
Member Author

villebro commented Dec 8, 2023

/testenv up

Copy link
Contributor

github-actions bot commented Dec 8, 2023

@villebro Ephemeral environment spinning up at http://35.93.27.117:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@villebro
Copy link
Member Author

villebro commented Dec 8, 2023

@michael-s-molina @sfirke I've created a chart that demonstrates how this feature works: Truncated numerical x-axis I may add a new integration test to ensure the e2e flow works, but in the meantime I think the PR should be ready for review.

@@ -26,6 +26,7 @@ export const DEFAULT_FORM_DATA: Partial<EchartsBubbleFormData> = {
logYAxis: false,
xAxisTitleMargin: 30,
yAxisTitleMargin: 30,
truncateXAxis: false,
Copy link
Member Author

@villebro villebro Dec 8, 2023

Choose a reason for hiding this comment

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

Note that for bubble chart, I'm assuming truncation is disabled by default, as it's more likely people want to see it starting from origo in this case (having a truncated x axis but a non-truncated y axis doesn't make sense for a scatter plot IMO).

Copy link
Member

Choose a reason for hiding this comment

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

I don't necessarily agree, I think it's lower-risk to have truncation as the default here too. If the user's data is suited for showing the origin, then I think it will look okay with the default truncation, but the converse is not true.

The x-axis in a scatterplot could be temperature, sales $, counts, or anything else where the difference between zero and the min value is greater than the range between min and max [which is the condition where truncation is most important].

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm fine with using truncateXAxis: true as the global default. Any objections @michael-s-molina ? I assume we can do this later without blocking the release, as it's a pretty minor change IMO..

Copy link
Member

Choose a reason for hiding this comment

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

No objection. We can cherry-pick the change for the next patch.

@michael-s-molina michael-s-molina changed the title feat(plugin-chart-echarts): support truncated numeric x-axis fix(plugin-chart-echarts): support truncated numeric x-axis Dec 8, 2023
@michael-s-molina
Copy link
Member

Thanks for the PR @villebro. While testing, I found a weird behavior when applying the bounds to temporal and categorial axes. I pushed a change to ignore the bounds for non-numerical axis and also improved the tooltip description to make it more clear that this control is only for numerical types.

Copy link

codecov bot commented Dec 8, 2023

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (d2adc85) 69.19% compared to head (cc82fcf) 69.19%.
Report is 1 commits behind head on master.

Files Patch % Lines
...gin-chart-echarts/src/Timeseries/transformProps.ts 14.28% 3 Missing and 3 partials ⚠️
.../plugin-chart-echarts/src/Bubble/transformProps.ts 66.66% 0 Missing and 1 partial ⚠️
...hart-echarts/src/MixedTimeseries/transformProps.ts 0.00% 1 Missing ⚠️
...tend/plugins/plugin-chart-echarts/src/controls.tsx 66.66% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master   #26215   +/-   ##
=======================================
  Coverage   69.19%   69.19%           
=======================================
  Files        1945     1945           
  Lines       75928    75937    +9     
  Branches     8453     8458    +5     
=======================================
+ Hits        52537    52544    +7     
- Misses      21207    21208    +1     
- Partials     2184     2185    +1     
Flag Coverage Δ
javascript 56.51% <47.05%> (+<0.01%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the fix @villebro.

@michael-s-molina michael-s-molina merged commit 07e5fe8 into apache:master Dec 8, 2023
30 checks passed
Copy link
Contributor

github-actions bot commented Dec 8, 2023

Ephemeral environment shutdown and build artifacts deleted.

@michael-s-molina michael-s-molina added v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch v3.1 Label added by the release manager to track PRs to be included in the 3.1 branch labels Dec 8, 2023
michael-s-molina added a commit that referenced this pull request Dec 8, 2023
Co-authored-by: Michael S. Molina <michael.s.molina@gmail.com>
michael-s-molina pushed a commit that referenced this pull request Dec 8, 2023
Co-authored-by: Michael S. Molina <michael.s.molina@gmail.com>
(cherry picked from commit 07e5fe8)
@sfirke
Copy link
Member

sfirke commented Dec 8, 2023

This is a nice enhancement, thank you @villebro 🙌 !

@villebro villebro deleted the villebro/xaxis-trunc branch December 8, 2023 17:03
@john-bodley
Copy link
Member

Out of interest @villebro where was the original [1900, 2017) range defined?

@villebro
Copy link
Member Author

villebro commented Dec 8, 2023

@john-bodley in the screenshot you should be able to see that the lower bound has been set manually to 1900, with the upper bound being left undefined (=autodetect).

image

@john-bodley
Copy link
Member

@villebro sorry by bad I was looking at the wrong screenshot. I meant the [0, 2,500) interval which resulted in the original "dirac delta"-like behavior.

@villebro
Copy link
Member Author

villebro commented Dec 8, 2023

@villebro sorry by bad I was looking at the wrong screenshot. I meant the [0, 2,500) interval which resulted in the original "dirac delta"-like behavior.

@john-bodley oh, that range wasn't explicitly defined anywhere, that's just what ECharts defaults to unless the bounds are either overridden or set to dataMin and dataMax.

@sadpandajoe
Copy link
Contributor

@villebro I think this pr might have a weird affect on current echart bubble charts. I've documented the issue here: #26242.

josedev-union pushed a commit to Ortege-xyz/studio that referenced this pull request Jan 22, 2024
…6215)

Co-authored-by: Michael S. Molina <michael.s.molina@gmail.com>
(cherry picked from commit 07e5fe8)
@mistercrunch mistercrunch added 🍒 3.0.3 🍒 3.0.4 🍒 3.1.0 🍒 3.1.1 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels labels Mar 8, 2024
sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 2024
…6215)

Co-authored-by: Michael S. Molina <michael.s.molina@gmail.com>
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 v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch v3.1 Label added by the release manager to track PRs to be included in the 3.1 branch 🍒 3.0.3 🍒 3.0.4 🍒 3.1.0 🍒 3.1.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bar chart with numeric x-axis always starts x-axis at zero regardless of data
6 participants