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 stacked areas overflow on null values when connectNulls is true #19602

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alxnddr
Copy link

@alxnddr alxnddr commented Feb 8, 2024

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

Minimal reproduction
Fix on the preview branch

The pull request fixes the logic of computing stacked on points of stacked areas which fixes areas overflow on null values when connectNulls is true.

Stacked on value can be NaN when:

  1. When the value of series exists but there is no series below it or the series it is stacked on do not have suitable values according to stackStrategy
  2. When the value of series itself is NaN

The getStackedOnPoint function behaved the same in both scenarios, when the stacked on value is NaN it decided that the area should be stacked on the valueStart of the series coordinates. However, when the value of series itself is NaN the stacked on value should be NaN as well for connectNulls option to work correctly and just connect the line without dropping it to the start of coordinates.

Fixed issues

Closes #19598
Closes #17135

Details

There is an existing test case area-stack.html which has a chart affected by the issue.

Before: What was the problem?

Yellow and blue areas have enabled connectNulls so the lines in the middle are connected but areas drop to the start of coordinates.

Screenshot 2024-02-08 at 4 29 21 PM

After: How does it behave after the fixing?

After the fix not only the lines are connected but also areas.

Screenshot 2024-02-08 at 4 28 39 PM

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

area-stack.html -> line break

Others

Merging options

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

Other information

Copy link

echarts-bot bot commented Feb 8, 2024

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.

@helgasoft
Copy link

The purpose of stacked lines is to show totals. This PR would make totals at all red points incorrect.
I think responsibility for such "fixes" belongs to the developer, not to the charting library.
Solution is simple - preprocess your data by removing or approximating missing values, then ECharts will gladly display what you want to see.

image

@alxnddr
Copy link
Author

alxnddr commented Feb 13, 2024

@helgasoft thanks for your feedback. In my pull request I addressed areas stacking when connectNulls is enabled so that it literally connects polygon lines on null values instead of dropping the area polygon to zero which does not look correct:
Screenshot 2024-02-08 at 4 29 21 PM

Manually interpolating values would break the step option behavior, and in addition to it supporting the smooth option would be a disproportionate effort to implement with custom interpolation.

To summarize

  • the existing behavior is not correct which was was reported by a few people
  • the PR changes make connectNulls work as it is called — connect line and polygon on null values
  • there is no workaround that does not limit users from using other features of ECharts

Based on these points, I believe this change is valid to be merged to the library code.

@helgasoft
Copy link

addressed areas stacking when connectNulls is enabled

I think connectNulls should not be enabled in stacked area charts since it makes ECharts display fake totals.

Here are the two methods of data preprocessing - approximate or remove missing values, Demo Code
The developer would choose which one is more appropriate, or a combination of the two.

image
image

Copy link
Contributor

github-actions bot commented Feb 14, 2024

The changes brought by this PR can be previewed at: https://echarts.apache.org/examples/editor?version=PR-19602@536da2c

@alxnddr alxnddr force-pushed the fix/connect-nulls-on-stacked-areas-causes-overflow branch from e4ed74c to 194a793 Compare February 15, 2024 05:11
@alxnddr alxnddr force-pushed the fix/connect-nulls-on-stacked-areas-causes-overflow branch from 194a793 to 536da2c Compare February 15, 2024 05:13
@pull-request-size pull-request-size bot added size/S and removed size/M labels Feb 15, 2024
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.

While I do agree with @helgasoft that stacked null values make the sum value misleading, I personally think this PR provides a much better solution than our current implementation. And the viewer can judge from the broken area to see that you can't take y values as sums, which I think should be better if the developers don't know how or don't bother to remove null values. Let's see what others think about this feature.

@helgasoft
Copy link

A strong argument in favor would be to find out whether another charting library (highcharts, amcharts, ?) has taken a chance on implementing it.

@alxnddr alxnddr force-pushed the fix/connect-nulls-on-stacked-areas-causes-overflow branch from f1a67c7 to 934cb6a Compare March 2, 2024 20:56
@visakadev
Copy link

visakadev commented May 10, 2024

addressed areas stacking when connectNulls is enabled

I think connectNulls should not be enabled in stacked area charts since it makes ECharts display fake totals.

Here are the two methods of data preprocessing - approximate or remove missing values, Demo Code The developer would choose which one is more appropriate, or a combination of the two.

image image

approximate solution fixed my problem: 💯 👍

DEMO codesandbox

The data preprocessing - approximate solution fixed my problem:

In my case, when rendering the area range (minimum value, maximum value), and latest, I used the solution shown in the code before and applied a function to approximate the values.

Before: 👎
image

After approximate null values 👍
image

I added a custom tooltip to render the original value instead of the approximate value.
image
image

image

@admsev
Copy link

admsev commented May 25, 2024

@gooroodev review

@gooroodev
Copy link

1. Summary of Changes

The pull request addresses an issue where stacked areas overflow on null values when the connectNulls option is set to true. The changes involve updating the getStackedOnPoint function in src/chart/line/helper.ts to handle null values more gracefully by:

  • Separating the retrieval of stackedOverValue and stackResultValue.
  • Adding a condition to check for stackedOverValue and stackResultValue to determine if stackedOverValue should be set to dataCoordInfo.valueStart.

2. Issues, Bugs, or Typos

Issue 1: Unnecessary Check on dataCoordInfo.stacked

The condition dataCoordInfo.stacked && isNaN(stackResultValue) is redundant since dataCoordInfo.stacked is already checked before retrieving stackResultValue.

Proposed Improvement:
Remove the redundant dataCoordInfo.stacked check in the condition.

-    if (isNaN(stackedOverValue) && !(dataCoordInfo.stacked && isNaN(stackResultValue))) {
+    if (isNaN(stackedOverValue) && !isNaN(stackResultValue)) {

3. General Review of Code Quality and Style

Positive Aspects:

  • The code changes are clear and well-targeted at solving the specific issue.
  • Variable names are meaningful and improve readability.

Suggestions for Improvement:

  • Consistency in formatting: Ensure consistent use of indentation and spacing.
  • Comments: Adding comments to explain the logic, especially the condition handling, would improve maintainability.

Example of Improved Code with Comments:

export function getStackedOnPoint(
    dataCoordInfo: DataCoordInfo,
    coordSys: CoordinateSystem,
    data: SeriesData,
    idx: number
) {
    let stackedOverValue = NaN;
    let stackResultValue = NaN;

    // Retrieve stacked values if stacking is enabled
    if (dataCoordInfo.stacked) {
        stackedOverValue = data.get(
            data.getCalculationInfo('stackedOverDimension'),
            idx
        ) as number;
        stackResultValue = data.get(
            data.getCalculationInfo('stackResultDimension'),
            idx
        ) as number;
    }

    // If stackedOverValue is NaN but stackResultValue is valid, use valueStart
    if (isNaN(stackedOverValue) && !isNaN(stackResultValue)) {
        stackedOverValue = dataCoordInfo.valueStart;
    }

    const baseDataOffset = dataCoordInfo.baseDataOffset;
    const stackedData = [];
    stackedData[baseDataOffset] = data.get(dataCoordInfo.baseDim, idx);
    stackedData[1 - baseDataOffset] = stackedOverValue;

    return coordSys.dataToPoint(stackedData);
}

Overall, the pull request effectively addresses the issue, but minor improvements in condition handling and code comments would enhance its quality.

Yours, Gooroo.dev. To receive reviews automatically, install Github App

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