Copilot commented on code in PR #21604:
URL: https://github.com/apache/echarts/pull/21604#discussion_r3182280803
##########
src/processor/dataStack.ts:
##########
@@ -170,3 +198,104 @@ function calculateStack(stackInfoList: StackInfo[]) {
});
});
}
+
+function calculateStackTotalMaps(stackInfoList: StackInfo[]) {
+ const stackTotalMaps: StackTotalMaps = {
+ byIndex: createHashMap<StackTotal, string | number>(),
+ byDimension: createHashMap<StackTotal, string | number>()
+ };
+
+ for (let i = 0; i < stackInfoList.length; i++) {
+ const stackInfo = stackInfoList[i];
+ const data = stackInfo.data;
+
+ 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
+ );
+ }
Review Comment:
calculateStackTotalMaps always populates the byIndex totals map even when
the stack group is stacked-by-dimension (isStackedByIndex is false for all
series), but normalizeStackValue will only read one of the maps. Consider
skipping the unused map (or building totals based on the stack mode of the
group) to avoid extra per-point work and allocations when normalizing
stacked-by-dimension series.
##########
src/processor/dataStack.ts:
##########
@@ -170,3 +198,104 @@ function calculateStack(stackInfoList: StackInfo[]) {
});
});
}
+
+function calculateStackTotalMaps(stackInfoList: StackInfo[]) {
+ const stackTotalMaps: StackTotalMaps = {
+ byIndex: createHashMap<StackTotal, string | number>(),
+ byDimension: createHashMap<StackTotal, string | number>()
+ };
+
+ for (let i = 0; i < stackInfoList.length; i++) {
+ const stackInfo = stackInfoList[i];
+ const data = stackInfo.data;
+
+ 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
+ );
+ }
+ }
+ }
+
+ return stackTotalMaps;
+}
+
+function addStackTotal(stackTotalMap: StackTotalMap, key: string | number,
value: number) {
+ const total = stackTotalMap.get(key) || stackTotalMap.set(key, {
+ all: 0,
+ positive: 0,
+ negative: 0
+ });
+
+ total.all = addSafe(total.all, value);
+ if (value > 0) {
+ total.positive = addSafe(total.positive, value);
+ }
+ else if (value < 0) {
+ total.negative = addSafe(total.negative, value);
+ }
+}
+
+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;
+}
Review Comment:
The added unit tests only cover stackNormalize with positive values and the
default stackStrategy, but the new normalization logic adds additional branches
(getStackTotal for 'all'/'positive'/'negative'/'samesign', plus NaN/zero-total
handling). Please add unit tests that exercise stackNormalize with negative
values and at least one non-default stackStrategy to ensure those code paths
are validated and protected from regressions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
