feat: pie-chart support negative value #15095

ssthouse · 2021-06-07T08:01:08Z

Brief Information

This pull request is in the type of:

bug fixing
new feature
others

What does this PR do?

Support minus value in Pie Chart.

Fixed issues

echarts 饼图负数问题 #15055

Details

Before: What was the problem?

the minus value slice fillup the entire pie chart;

After: How is it fixed in this PR?

Misc

The API has been changed (apache/echarts-doc#xxx).
This PR depends on ZRender changes (ecomfe/zrender#xxx).

Related test cases or examples to use the new APIs

NA.

add test file: pie-minus-value.html in test folder;

Others

Merging options

Please squash the commits into a single one when merge.

Other information

echarts-bot · 2021-06-07T08:01:10Z

Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout the coding standard and Wiki about How to make a pull request.

src/chart/pie/pieLayout.ts

src/chart/pie/PieSeries.ts

Ovilia · 2021-06-07T10:35:56Z

@ssthouse Could you try to filter negative data with pie series in processor/dataFilter.ts? If it works, you don't need to change pieView and it would be simpler. Please be careful when checking the legend of negative data.

ssthouse · 2021-06-07T11:00:58Z

@ssthouse Could you try to filter negative data with pie series in processor/dataFilter.ts? If it works, you don't need to change pieView and it would be simpler. Please be careful when checking the legend of negative data.

I'll try~

ssthouse · 2021-06-07T11:22:43Z

@Ovilia hi, do you mean adding filter logic like this?

So I can discard the changed I've made in src/chart/pie/pieLayout.ts and src/chart/pie/PieSeries.ts

pissang · 2021-06-08T03:26:07Z

@ssthouse Should register a new processor to filter the negative data. See https://github.com/apache/echarts/blob/master/src/chart/pie/install.ts#L36 about how to register a processor

ssthouse · 2021-06-08T04:56:36Z

@pissang hi, I create a new filter: negativeDataFilter.ts and register it in pie/install.ts, please check~

plainheart · 2021-06-08T05:16:13Z

It seems this will break the case in which all the number is negative. Besides, if the negative value was considered as 0, how does the tooltip show? Should it be 0(as the pie shows) or the original negative value like -1548?

ssthouse · 2021-06-08T06:21:18Z

It seems this will break the case in which all the number is negative. Besides, if the negative value was considered as 0, how does the tooltip show? Should it be 0(as the pie shows) or the original negative value like -1548?

@plainheart hi, In my previous edition(see the first two commits of this PR).
the negative value is considered as 0 when calculating pie slices. And the tooltip shows the negative value label; like below:

according to @Ovilia and @pissang's suggestion, I moved the filter logic to processor/negativeDataFilter( see the 3rd commit of this PR).
and in this edition, the negative value will only be shown in legend and not in pie slices. like below:

I'm not sure which result is expected since they're both reasonable in some way，hope you guys can give me a conclusion😂

Ovilia · 2021-06-08T12:12:27Z

I think it's a little better if not show in the tooltip but show in legend.

src/processor/negativeDataFilter.ts

src/chart/pie/pieLayout.ts

Ovilia · 2021-06-08T12:15:54Z

test/pie-minus-value.html

+ center: ["50%", "60%"],
+ selectedMode: "single",
+ data: [
+ { value: 335, name: "直接访问" },


As @plainheart suggested, I think we should add a test case that all data are negative.

When all the data are negative, the pie chart is empty.
I'm not sure if this is the expected result 🐒

When all the data are negative, the pie chart is empty.
I'm not sure if this is the expected result 🐒

At least, it's unexpected for me. This will break the previous behavior (See below). And I think it's meaningful when all value is negative. In that case, considering them as 0 or filtering them may be not a good way.

If the canvas is blank, it looks like there is an error to the users. So perhaps we could display an empty light gray circle when no data (sum is zero or all is '-' or negative). We should also provide options to configure the color.
The legend color doesn't look right to me. They should have different colors for different series.

@Ovilia this sounds reasonable, I'll give it a try~

display an empty light gray circle when no data

For the color issue, all three legends' are in blue because they are all filtered by negativeDataFilter. And the filtered legend's default color is blue

ssthouse · 2021-06-16T07:48:40Z

@ssthouse Awesome! Can we have the ability to configure if display this empty circle and the style. Perhaps the options can be:
showEmptyCircle: true,
emptyCircleStyle: {
  // same with itemStyle
}

@pissang Yes, sure~

I have a small question that may need your help. 🙏

About the option emptyCircleStyle. Currently, I implement the light-gray circle by creating a simple graphic.Sector, and the type of itemStyle (ItemStyleOption) seems can not be directly applied to Sector.

I checked the sector's API and found it accepts PathStyleProps, maybe we can expose emptyCircleStyle: PathStyleProps ?

pissang · 2021-06-16T08:13:00Z

@ssthouse Here is a common way to set style from the option:

sector.useStyle(seriesModel.getModel('emptyCircleStyle').getItemStyle());

Some notes:

Need to use useStyle here so the style properties can be reset to default when developers didn't set it in the new options. Meanwhile setStyle will merge the new style to the existing style.
seriesModel.getModel('emptyCircleStyle') get a model subset which only includes emptyCircleStyle option.
getItemStyle get the full mapped style object from the option. It includes fill and stroke. Meanwhile, there are other two methods that can also get a mapped style object. getLineStyle which only includes stroke. getAreaStyle which only includes fill.

ssthouse · 2021-06-16T09:10:32Z

@pissang Thanks for your suggestions, really helpful!

I have added the config options, please check~

pissang · 2021-06-16T09:29:38Z

src/processor/negativeDataFilter.ts

+ const legendModels = ecModel.findComponents({
+ mainType: 'legend'
+ });
+ if (!legendModels || !legendModels.length) {


I think it's not necessary to check if there is legend?

pissang · 2021-06-16T09:32:03Z

src/chart/pie/PieView.ts

@@ -242,6 +243,29 @@ class PieView extends ChartView {
 }
 }

+ // remove empty-circle(which is not instance of PiePiece) if it exists
+ group.children().forEach(child => {
+ if (!(child instanceof PiePiece)) {


It's not a good way to check if it's the empty-circle element by checking it's not a piece of the pie. It may have issues if we add other elements that are not a piece of the pie further.

A better way is to save this empty-circle element as a private property of PieView. It's simpler and more robust.

…Circle method

pissang · 2021-06-17T01:45:44Z

LGTM now. @Ovilia @plainheart Any more comments?

plainheart

Thanks for the work. It should be okay generally. But I can't keep confident about the final process: filter the negative values.

Comparing to current behavior, this result looks more expected for me,

That is, leave the negative value there and its percentage is 0.

And if all values are negative, display them as the same as the positive ones. (BTW, this is also the behavior before this PR's changes.)

src/chart/pie/PieView.ts

pissang · 2021-06-17T06:00:20Z

@plainheart About the negative value filtering. I can accept it mainly because highcharts is using the same strategy. But I'm also not sure if there are developers using this "bug" as a "feature" to get the result they want. If so this change may break it and we need an extra option to choose not to filter these negative values.

ssthouse · 2021-06-18T02:39:19Z

@pissang Should I add this config now or we wait for developers‘ feedback after release?

pissang · 2021-06-24T06:58:22Z

@ssthouse I think we can wait for the developers' feedback.

Seems there is still a blocking change request from @Ovilia

Ovilia

LGTM

echarts-bot · 2021-06-25T04:23:58Z

Congratulations! Your PR has been merged. Thanks for your contribution! 👍

yangkai-bj · 2021-09-15T08:00:14Z

在Pie中强制过滤负数,是否简单粗暴了一些,在饼图和圆环图中负数确实导致图形绘制的问题,但玫瑰图可以真实反映负数,是否可以将PIE中"是否过滤负值"作为一个用户选项.
日常涉及规模数据的时候一般都是非负数,使用Pie绘制很好理解,但涉及增量的占比,负增数据无法避免,饼图和圆环图都无法绘制,往往需要对原始数据进行变形(将所有值+abs(最小负值))后绘制,绘制后的图表让人很难理解,5.1.2以前的玫瑰图(roseType: "area")可以真实体现原始数据中的负数.

yangkai-bj · 2021-09-15T08:00:34Z

pissang · 2021-09-15T08:09:59Z

@yangkai-bj 谢谢建议，欢迎 PR 增加该配置

echarts-bot bot added PR: awaiting review PR: first-time contributor labels Jun 7, 2021

pull-request-size bot added the size/M label Jun 7, 2021

pissang requested a review from Ovilia June 7, 2021 09:24

Ovilia requested changes Jun 7, 2021

View reviewed changes

src/chart/pie/pieLayout.ts Outdated Show resolved Hide resolved

src/chart/pie/PieSeries.ts Outdated Show resolved Hide resolved

echarts-bot bot added PR: revision needed PR: awaiting review and removed PR: awaiting review PR: revision needed labels Jun 7, 2021

pull-request-size bot added size/S and removed size/M labels Jun 7, 2021

pull-request-size bot added size/M and removed size/S labels Jun 8, 2021

Ovilia requested changes Jun 8, 2021

View reviewed changes

echarts-bot bot added PR: revision needed PR: awaiting review and removed PR: awaiting review PR: revision needed labels Jun 8, 2021

ssthouse force-pushed the fix/pie-chart-minus-value branch from 3b15503 to 21e2f65 Compare June 8, 2021 14:22

ssthouse changed the title ~~feat: pie-chart support minus value~~ feat: pie-chart support negative value Jun 8, 2021

feat: pie-chart support minus value

1bfca84

pissang added this to the 5.2.0 milestone Jun 16, 2021

feat: make emptyCircle's visibility and style configurable

c30a730

pissang requested changes Jun 16, 2021

View reviewed changes

echarts-bot bot added PR: revision needed PR: awaiting review and removed PR: awaiting review PR: revision needed labels Jun 16, 2021

fix: remove useless logic in negativeDataFilter & improve clear empty…

876c7b3

…Circle method

ssthouse force-pushed the fix/pie-chart-minus-value branch from ed3ad28 to 876c7b3 Compare June 16, 2021 09:48

pissang previously approved these changes Jun 17, 2021

View reviewed changes

plainheart reviewed Jun 17, 2021

View reviewed changes

src/chart/pie/PieView.ts Outdated Show resolved Hide resolved

fix: simplify sector creation

17a11c5

ssthouse dismissed pissang’s stale review via 17a11c5 June 17, 2021 04:43

plainheart approved these changes Jun 17, 2021

View reviewed changes

ssthouse requested a review from Ovilia June 24, 2021 14:39

Ovilia approved these changes Jun 25, 2021

View reviewed changes

pissang merged commit 223e636 into apache:master Jun 25, 2021

echarts-bot bot removed the PR: awaiting review label Jun 25, 2021

plainheart mentioned this pull request May 10, 2023

fix(pie): display empty circle correctly #18604

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: pie-chart support negative value #15095

feat: pie-chart support negative value #15095

ssthouse commented Jun 7, 2021

echarts-bot bot commented Jun 7, 2021

Ovilia commented Jun 7, 2021

ssthouse commented Jun 7, 2021

ssthouse commented Jun 7, 2021

pissang commented Jun 8, 2021

ssthouse commented Jun 8, 2021

plainheart commented Jun 8, 2021

ssthouse commented Jun 8, 2021

Ovilia commented Jun 8, 2021

Ovilia Jun 8, 2021

ssthouse Jun 8, 2021

plainheart Jun 9, 2021 •

edited

Loading

Ovilia Jun 11, 2021

ssthouse Jun 14, 2021

ssthouse commented Jun 16, 2021

pissang commented Jun 16, 2021 •

edited

Loading

ssthouse commented Jun 16, 2021

pissang Jun 16, 2021

ssthouse Jun 16, 2021

pissang Jun 16, 2021

ssthouse Jun 16, 2021

pissang commented Jun 17, 2021 •

edited

Loading

plainheart left a comment

pissang commented Jun 17, 2021 •

edited

Loading

ssthouse commented Jun 18, 2021

pissang commented Jun 24, 2021 •

edited

Loading

Ovilia left a comment

echarts-bot bot commented Jun 25, 2021

yangkai-bj commented Sep 15, 2021

yangkai-bj commented Sep 15, 2021

pissang commented Sep 15, 2021

feat: pie-chart support negative value #15095

feat: pie-chart support negative value #15095

Conversation

ssthouse commented Jun 7, 2021

Brief Information

What does this PR do?

Fixed issues

Details

Before: What was the problem?

After: How is it fixed in this PR?

Misc

Related test cases or examples to use the new APIs

Others

Merging options

Other information

echarts-bot bot commented Jun 7, 2021

Ovilia commented Jun 7, 2021

ssthouse commented Jun 7, 2021

ssthouse commented Jun 7, 2021

pissang commented Jun 8, 2021

ssthouse commented Jun 8, 2021

plainheart commented Jun 8, 2021

ssthouse commented Jun 8, 2021

Ovilia commented Jun 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plainheart Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssthouse commented Jun 16, 2021

pissang commented Jun 16, 2021 • edited Loading

ssthouse commented Jun 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pissang commented Jun 17, 2021 • edited Loading

plainheart left a comment

Choose a reason for hiding this comment

pissang commented Jun 17, 2021 • edited Loading

ssthouse commented Jun 18, 2021

pissang commented Jun 24, 2021 • edited Loading

Ovilia left a comment

Choose a reason for hiding this comment

echarts-bot bot commented Jun 25, 2021

yangkai-bj commented Sep 15, 2021

yangkai-bj commented Sep 15, 2021

pissang commented Sep 15, 2021

plainheart Jun 9, 2021 •

edited

Loading

pissang commented Jun 16, 2021 •

edited

Loading

pissang commented Jun 17, 2021 •

edited

Loading

pissang commented Jun 17, 2021 •

edited

Loading

pissang commented Jun 24, 2021 •

edited

Loading