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(pie): Use actual area measure instead of radius in Rose Chart #15674 #18978

Closed
wants to merge 5 commits into from

Conversation

lulu0119
Copy link

@lulu0119 lulu0119 commented Aug 9, 2023

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

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 After
60851d1decfa9cf8b14fd08ab00b45b dc49f5a2228ca171c8b061db51c7f03

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.

  • This PR doesn't relate to document changes
  • The document should be updated later
  • The document changes have been made in apache/echarts-doc#xxx

Misc

ZRender Changes

  • This PR depends on ZRender changes (ecomfe/zrender#xxx).

Related test cases or examples to use the new APIs

N.A.

Others

Merging options

  • Please squash the commits into a single one when merging.

Other information

…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.
@echarts-bot
Copy link

echarts-bot bot commented Aug 9, 2023

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.

⚠️ MISSING DOCUMENT INFO: Please make sure one of the document options are checked in this PR's description. Search "Document Info" in the description of this PR. This should be done either by the author or the reviewers of the PR.

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.

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

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.

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.

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.
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.

Demo

With data

data: [
        { value: 10, name: 'rose 1' },
        { value: 20, name: 'rose 2' },
        { value: 30, name: 'rose 3' },
        { value: 40, name: 'rose 4' }
      ]
image

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.

@lulu0119
Copy link
Author

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.

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.

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.

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.

2 participants