[codex] Enable normalized stacked area charts#21604
Open
Conversation
Line stack values can now opt into stackNormalize to render 100% stacked area charts while preserving the default stacked line behavior. The stack processor normalizes only all-line stack groups and keeps existing stack strategy and stack order semantics. Unit coverage and an HTML visual case lock the behavior, and AGENTS records that dist output is generated during npm publish. Constraint: dist files are generated during npm publish and must not be manually edited Rejected: Modify dist or generated declaration output directly | generated artifacts should come from the release flow Confidence: high Scope-risk: moderate Reversibility: clean Tested: npx jest --config test/ut/jest.config.cjs --coverage=false test/ut/spec/series/lineStack.test.ts Tested: npm run checktype Tested: npx eslint src/processor/dataStack.ts src/chart/line/LineSeries.ts test/ut/spec/series/lineStack.test.ts Tested: git diff --check on changed source, HTML, test, and AGENTS files Not-tested: npm run test:dts:fast; existing NodeNext declaration errors block the broader d.ts suite before this option is exercised
The normalized stacked area HTML case now uses 100 deterministic data points so the preview better represents a realistic dense area chart while preserving the same top-boundary-at-one verification shape. Constraint: Visual fixture should stay deterministic for screenshot review Rejected: Use random data | screenshots would drift between runs Confidence: high Scope-risk: narrow Reversibility: clean Tested: npx jest --config test/ut/jest.config.cjs --coverage=false test/ut/spec/series/lineStack.test.ts Tested: git diff --check -- test/area-stack.html Tested: Browser preview at test/area-stack.html with 100 data points Not-tested: broader visual regression suite
|
Thanks for your contribution! The pull request is marked to be To reviewers: If this PR is going to be described in the changelog in the future release, please make sure this PR has one of the following labels: This message is shown because the PR description doesn't contain the document related template. |
Contributor
|
The changes brought by this PR can be previewed at: https://echarts.apache.org/examples/editor?version=PR-21604@dd3cc53 |
There was a problem hiding this comment.
Pull request overview
This PR adds support for normalized stacked area/line charts by introducing a series.line.stackNormalize option and extending the stack processor to normalize each stacked value by the per-category stack total. It also adds both unit and HTML-based visual coverage for the new behavior.
Changes:
- Add
stackNormalize?: booleantoLineSeriesOption. - Extend
dataStackprocessing to optionally normalize stacked line values by the stack total. - Add a Jest unit test and an HTML visual test case for normalized stacked area rendering.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/processor/dataStack.ts |
Adds conditional normalization for stacked line series and refactors stack strategy checks. |
src/chart/line/LineSeries.ts |
Introduces the stackNormalize series option in typings. |
test/ut/spec/series/lineStack.test.ts |
Adds unit tests validating normalized vs regular stacking output values. |
test/area-stack.html |
Adds a 100-point normalized stacked area visual case for manual/screenshot review. |
AGENTS.md |
Adds repository-wide agent guidance (tests, screenshots, and generated dist/ policy). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
133
to
+136
| targetData.modify(dims, function (v0, v1, dataIndex) { | ||
| let sum = targetData.get(targetStackInfo.stackedDimension, dataIndex) as number; | ||
| let sum = normalizeStack | ||
| ? normalizeStackValue(stackInfoList, targetStackInfo, dataIndex, stackStrategy) | ||
| : targetData.get(targetStackInfo.stackedDimension, dataIndex) as number; |
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
Copilot review identified that normalized stacked area calculation rescanned every series for each point. The processor now precomputes per-stack totals by raw index and by stack dimension, with sign buckets reused by each stack strategy. A regression test locks the accepted all-series opt-in behavior. Constraint: Do not modify dist; release build artifacts are generated during npm publish Rejected: Keep per-point stack rescans | scales as series^2 * points for normalized stacks Confidence: high Scope-risk: moderate Reversibility: clean Directive: Keep stackNormalize denominators aligned with stackStrategy sign buckets when changing stack semantics Tested: npx jest --config test/ut/jest.config.cjs --coverage=false test/ut/spec/series/lineStack.test.ts Tested: npm run checktype Tested: npx eslint src/processor/dataStack.ts src/chart/line/LineSeries.ts test/ut/spec/series/lineStack.test.ts Tested: git diff --check src/processor/dataStack.ts test/ut/spec/series/lineStack.test.ts Not-tested: Browser visual screenshot against a fresh source bundle; avoided regenerating dist per repo rule
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+212
to
+227
| for (let dataIndex = 0, len = data.count(); dataIndex < len; dataIndex++) { | ||
| const value = data.get(stackInfo.stackedDimension, dataIndex) as number; | ||
|
|
||
| if (isNaN(value)) { | ||
| continue; | ||
| } | ||
|
|
||
| addStackTotal(stackTotalMaps.byIndex, data.getRawIndex(dataIndex), value); | ||
|
|
||
| if (stackInfo.stackedByDimension) { | ||
| addStackTotal( | ||
| stackTotalMaps.byDimension, | ||
| data.get(stackInfo.stackedByDimension, dataIndex) as number, | ||
| value | ||
| ); | ||
| } |
Comment on lines
+250
to
+289
| function normalizeStackValue( | ||
| stackTotalMaps: StackTotalMaps, | ||
| targetStackInfo: StackInfo, | ||
| dataIndex: number, | ||
| stackStrategy: StackSeriesOption['stackStrategy'] | ||
| ) { | ||
| const rawValue = targetStackInfo.data.get(targetStackInfo.stackedDimension, dataIndex) as number; | ||
|
|
||
| if (isNaN(rawValue)) { | ||
| return NaN; | ||
| } | ||
|
|
||
| const stackTotalMap = targetStackInfo.isStackedByIndex | ||
| ? stackTotalMaps.byIndex | ||
| : stackTotalMaps.byDimension; | ||
| const key = targetStackInfo.isStackedByIndex | ||
| ? targetStackInfo.data.getRawIndex(dataIndex) | ||
| : targetStackInfo.data.get(targetStackInfo.stackedByDimension, dataIndex) as number; | ||
| const totalInfo = stackTotalMap.get(key); | ||
| const total = totalInfo && getStackTotal(totalInfo, rawValue, stackStrategy); | ||
| return total ? rawValue / Math.abs(total) : 0; | ||
| } | ||
|
|
||
| function getStackTotal( | ||
| totalInfo: StackTotal, | ||
| targetValue: number, | ||
| stackStrategy: StackSeriesOption['stackStrategy'] | ||
| ) { | ||
| if (stackStrategy === 'all') { | ||
| return totalInfo.all; | ||
| } | ||
| else if (stackStrategy === 'positive') { | ||
| return totalInfo.positive; | ||
| } | ||
| else if (stackStrategy === 'negative') { | ||
| return totalInfo.negative; | ||
| } | ||
|
|
||
| return targetValue >= 0 ? totalInfo.positive : totalInfo.negative; | ||
| } |
Follow-up review noted that the normalized stack total cache still did extra work by filling both index and dimension maps. The processor now builds each cache lazily for the stack mode that actually needs it, preserving mixed-mode safety without doing unused per-point aggregation. Additional line stack tests cover negative normalized values and the non-default all stack strategy. Constraint: Do not modify dist; release build artifacts are generated during npm publish Rejected: Build both total maps eagerly | wastes work and allocations for the common single-mode stack group Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep stack total cache construction lazy if future stack modes add more denominator keys Tested: npx jest --config test/ut/jest.config.cjs --coverage=false test/ut/spec/series/lineStack.test.ts Tested: npm run checktype Tested: npx eslint src/processor/dataStack.ts src/chart/line/LineSeries.ts test/ut/spec/series/lineStack.test.ts Tested: git diff --check src/processor/dataStack.ts test/ut/spec/series/lineStack.test.ts Not-tested: Browser visual screenshot against a fresh source bundle; avoided regenerating dist per repo rule
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+277
to
+293
| function getStackTotal( | ||
| totalInfo: StackTotal, | ||
| targetValue: number, | ||
| stackStrategy: StackSeriesOption['stackStrategy'] | ||
| ) { | ||
| if (stackStrategy === 'all') { | ||
| return totalInfo.all; | ||
| } | ||
| else if (stackStrategy === 'positive') { | ||
| return totalInfo.positive; | ||
| } | ||
| else if (stackStrategy === 'negative') { | ||
| return totalInfo.negative; | ||
| } | ||
|
|
||
| return targetValue >= 0 ? totalInfo.positive : totalInfo.negative; | ||
| } |
| stackTotalMap, | ||
| isStackedByIndex | ||
| ? data.getRawIndex(dataIndex) | ||
| : data.get(stackInfo.stackedByDimension, dataIndex) as number, |
Follow-up review asked for direct coverage of positive and negative stack strategy normalization and called out that stacked-by-dimension cache keys are not number-only. The tests now lock mixed-sign behavior for both strategies, and the total-cache key type is explicit about accepting string or number values. Constraint: Do not modify dist; release build artifacts are generated during npm publish Rejected: Rely on samesign/all tests for positive and negative branches | leaves dedicated strategy branches unprotected Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep normalized stack cache keys aligned with ParsedValue ordinal raw values when changing dimension handling Tested: npx jest --config test/ut/jest.config.cjs --coverage=false test/ut/spec/series/lineStack.test.ts Tested: npm run checktype Tested: npx eslint src/processor/dataStack.ts src/chart/line/LineSeries.ts test/ut/spec/series/lineStack.test.ts Tested: git diff --check src/processor/dataStack.ts test/ut/spec/series/lineStack.test.ts Not-tested: Browser visual screenshot against a fresh source bundle; avoided regenerating dist per repo rule
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
series.line.stackNormalizefor normalized stacked area chartsdist/is generated during npm publish and should not be edited manuallyValidation
npx jest --config test/ut/jest.config.cjs --coverage=false test/ut/spec/series/lineStack.test.tsnpm run checktypenpx eslint src/processor/dataStack.ts src/chart/line/LineSeries.ts test/ut/spec/series/lineStack.test.tsgit diff --checkon changed source, HTML, test, and AGENTS filesNotes
dist/was not modified; generated files are left to the npm publish flow.npm run test:dts:fastis currently blocked by existing NodeNext declaration errors before this option is exercised.