-
Notifications
You must be signed in to change notification settings - Fork 19.6k
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(pie): Use actual area measure instead of radius in Rose Chart #15674 #18978
Conversation
…che#15674 This pull request addresses the issue mentioned in apache#15674 by fixing the calculation of the slice radius in the Rose Chart. Currently, the radius of the sectors in the chart is mapped to the data, which can be misleading and result in disproportionate representation of the values. The proposed solution suggests calculating the slice radius using the square root of the value, rather than the value itself. This adjustment will ensure that the areas of the slices accurately reflect the data, providing a more consistent and visually accurate representation. By implementing this change, the resulting rendering of the Rose Chart will accurately represent the differences in data points, providing a more informative and visually appealing visualization. This pull request aims to improve the Rose Chart functionality and enhance the overall user experience.
Thanks for your contribution! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution. It's nice to see an old feature request being answered.
It seems that you want to name the new feature as 'area'
but this may cause incompatibility problem. So I would suggest not change the behavior of 'area'
while add a new roseType
value.
Besides, you need to take care of the code with roseType
. Search it and see if extra code is needed. For example, if (roseType !== 'area') {
around Line 144 in pieLayout.ts
. You should probably add your new value name.
Add test cases and run existing pie chart cases to make sure we are not breaking things down. See the wiki for more information.
: r | ||
r: roseType === 'area' | ||
// calculate square root of value and extent, and map to range between r0 and r | ||
? linearMap(Math.sqrt(value), extent.map(Math.sqrt), [r0, r]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although techinically speaking, negative values are not supported for pie charts, but I'm not sure if it will run here so make sure it is tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see code changes. Convert to need-modification state.
… cases apache#15674 This commit adds a new value 'scaledArea' to the roseType option in rose chart. When set to 'scaledArea', the radius is calculated based on the square root, and the ratio of the actual value is proportional to the ratio of the area occupied by different items. Additionally, new test cases have been added to demonstrate the effects of different roseType values. My testing shows that when the value is negative, the pie chart will lack that specific item, but it is unclear how the code handles it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With data
data: [
{ value: 10, name: 'rose 1' },
{ value: 20, name: 'rose 2' },
{ value: 30, name: 'rose 3' },
{ value: 40, name: 'rose 4' }
]
Although theoretically it is misleading to use area to represent the value, I also doubt in this case users will think the blue one is half of the green one. But I'm OK to have this feature to give developers another option.
BTW, this new design should not take the origin name 'area'
. Otherwise, there would be compatible problems. You should give a new name to your design and make sure it doesn't breaks anything down.
Thank you for your response to my pull request. I agree with your suggestion that the new design should avoid changing the existing name "area" to avoid compatibility issues. Following your advice, I have submitted a commit (20b51db) to fix the issue and named the new design "scaledArea" to avoid conflicts with existing names. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution! But I'm afraid I would like to vote against this feature as described in #18978 (review) . I'm afraid I have to close this PR. If you think the roseType is misleading, I would suggest not using it at all, instead of making the area representing the data. This is because based on previous expriences, users have instincts about how a chart represents data and with roseType or pie charts, it's more the case with angles than area.
Brief Information
This pull request is in the type of:
What does this PR do?
This pull request introduces a new feature that modifies the calculation of the slice radius in the Rose Chart. It addresses the issue mentioned in #15674 by using the square root of the value to accurately represent the data in the visualization.
Fixed issues
Details
Before: What was the problem?
The problem was that the radius of the sectors in the Rose Chart was mapped to the data, which resulted in misleading visualizations. Longer slices received more emphasis due to their larger area size, exaggerating the differences in values.
After: How does it behave after the fixing?
After the fixing, the slice radius is calculated using the square root of the value. This adjustment ensures that the areas of the slices accurately reflect the data, providing a more consistent and visually accurate representation. The differences in values are now accurately represented without exaggeration.
Document Info
One of the following should be checked.
Misc
ZRender Changes
Related test cases or examples to use the new APIs
N.A.
Others
Merging options
Other information