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: pie-chart support negative value #15095

Merged
merged 8 commits into from Jun 25, 2021

Conversation

ssthouse
Copy link
Contributor

@ssthouse ssthouse commented Jun 7, 2021

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

Details

Before: What was the problem?

the minus value slice fillup the entire pie chart;

After: How is it fixed in this PR?

image

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
Copy link

echarts-bot bot commented Jun 7, 2021

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 Outdated Show resolved Hide resolved
src/chart/pie/PieSeries.ts Outdated Show resolved Hide resolved
@Ovilia
Copy link
Contributor

Ovilia commented Jun 7, 2021

@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
Copy link
Contributor Author

ssthouse commented Jun 7, 2021

@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
Copy link
Contributor Author

ssthouse commented Jun 7, 2021

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

image

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

@pissang
Copy link
Contributor

pissang commented Jun 8, 2021

@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

@pull-request-size pull-request-size bot added size/M and removed size/S labels Jun 8, 2021
@ssthouse
Copy link
Contributor Author

ssthouse commented Jun 8, 2021

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

@plainheart
Copy link
Member

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
Copy link
Contributor Author

ssthouse commented Jun 8, 2021

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:

image


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:
image

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
Copy link
Contributor

Ovilia commented Jun 8, 2021

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

src/processor/negativeDataFilter.ts Outdated Show resolved Hide resolved
src/chart/pie/pieLayout.ts Outdated Show resolved Hide resolved
center: ["50%", "60%"],
selectedMode: "single",
data: [
{ value: 335, name: "直接访问" },
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

image

Copy link
Member

@plainheart plainheart Jun 9, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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 ssthouse changed the title feat: pie-chart support minus value feat: pie-chart support negative value Jun 8, 2021
@pissang pissang added this to the 5.2.0 milestone Jun 16, 2021
@ssthouse
Copy link
Contributor Author

@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 ?

image

@pissang
Copy link
Contributor

pissang commented Jun 16, 2021

@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
Copy link
Contributor Author

@pissang Thanks for your suggestions, really helpful!

I have added the config options, please check~

image

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -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)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

pissang
pissang previously approved these changes Jun 17, 2021
@pissang
Copy link
Contributor

pissang commented Jun 17, 2021

LGTM now. @Ovilia @plainheart Any more comments?

Copy link
Member

@plainheart plainheart left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
@pissang
Copy link
Contributor

pissang commented Jun 17, 2021

@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
Copy link
Contributor Author

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

@pissang
Copy link
Contributor

pissang commented Jun 24, 2021

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

Seems there is still a blocking change request from @Ovilia

@ssthouse ssthouse requested a review from Ovilia June 24, 2021 14:39
Copy link
Contributor

@Ovilia Ovilia left a comment

Choose a reason for hiding this comment

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

LGTM

@pissang pissang merged commit 223e636 into apache:master Jun 25, 2021
@echarts-bot
Copy link

echarts-bot bot commented Jun 25, 2021

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

@yangkai-bj
Copy link

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

@yangkai-bj
Copy link

TEST10 (2)

@pissang
Copy link
Contributor

pissang commented Sep 15, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants