Fix line sampling with dataZoom filterMode none#21601
Fix line sampling with dataZoom filterMode none#21601
Conversation
Line sampling previously used the processed series count as its sampling base. When dataZoom used filterMode none or empty, that count still represented the full dataset, so a narrow zoom could remain downsampled as if the whole dataset were visible. This bases the sampling rate on points contained by the current base-axis extent while keeping the no-sampling path out of the processor. The regression HTML case compares filter, none, empty, and raw modes around a narrow zoom window. Constraint: dataZoom filterMode none and empty intentionally keep full SeriesData rather than filtering rows before sampling Rejected: Change dataZoom filtering semantics for none/empty | would break the documented promise that data is not filtered Confidence: high Scope-risk: narrow Directive: Sampling rate should follow the visible base-axis extent, not only the post-filter SeriesData count Tested: npm run checktype -- --pretty false Tested: npm run lint -- --quiet src/processor/dataSample.ts Tested: test/line-sampling-dataZoom-filterMode.html via Chrome, PASS with none/empty counts matching raw Not-tested: Full 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. |
|
The changes brought by this PR can be previewed at: https://echarts.apache.org/examples/editor?version=PR-21601@d503a78 |
There was a problem hiding this comment.
Pull request overview
Adjusts the downsampling (sampling) processor so that when dataZoom.filterMode keeps the full SeriesData ('none' / 'empty'), the sampling rate is derived from how many points are currently within the base-axis visible extent, fixing over-downsampling when zoomed in (#21512).
Changes:
- Compute sampling rate using the count of points inside the current base-axis extent (instead of total post-filter
data.count()). - Skip the sampling processor when
sampling: 'none'. - Add an HTML regression case covering
filtervsnone/emptybehaviors withlttband no sampling.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/processor/dataSample.ts | Updates sampling-rate calculation to be based on base-axis visible extent; skips processor for sampling: 'none'. |
| test/line-sampling-dataZoom-filterMode.html | Adds a regression page to validate sampling behavior under different dataZoom.filterMode settings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot pointed out two hot-path costs in the dataZoom-aware sampling fix. The safe change is to reuse the base-axis scale and call scale.contain on already-parsed store values, avoiding Axis#containData parsing for every item. The suggested direct numeric extent check was tested but regressed filterMode none in the HTML case, so this keeps scale containment semantics while still removing repeated parsing. The processor also exits before scanning the dataset when the post-filter count already cannot exceed the pixel sampling size. Constraint: filterMode none relies on scale containment semantics during processing; direct scale extent comparison did not preserve the regression case Rejected: Direct min/max check from scale.getExtent() | made none+lttb downsample to 1002 points in the regression page Confidence: high Scope-risk: narrow Tested: npm run checktype -- --pretty false Tested: npm run lint -- --quiet src/processor/dataSample.ts Tested: git diff --check -- src/processor/dataSample.ts Tested: test/line-sampling-dataZoom-filterMode.html via Chrome, PASS after Copilot feedback changes Not-tested: Full visual regression suite
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot flagged that SeriesData.each still adds callback binding overhead in the visible-window count path. This keeps the existing scale containment semantics but reads parsed values through the underlying DataStore with a precomputed dimension index. Constraint: direct min/max checks from scale.getExtent did not preserve the filterMode none regression case Confidence: high Scope-risk: narrow Tested: npm run checktype -- --pretty false Tested: npm run lint -- --quiet src/processor/dataSample.ts Tested: git diff --check -- src/processor/dataSample.ts Tested: test/line-sampling-dataZoom-filterMode.html via Chrome, PASS with store-loop count Not-tested: Full visual regression suite
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot flagged that a collapsed base-axis pixel extent still triggered the visible-count scan before sampling was skipped by a non-finite rate. This exits before scanning when the calculated sampling size is non-finite or non-positive. Constraint: zero-size chart containers can occur transiently during layout or resize Confidence: high Scope-risk: narrow Tested: npm run checktype -- --pretty false Tested: npm run lint -- --quiet src/processor/dataSample.ts Tested: git diff --check -- src/processor/dataSample.ts Tested: test/line-sampling-dataZoom-filterMode.html via Chrome, PASS with zero-size guard Not-tested: Full visual regression suite
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot review identified two edge cases in the sampling count path: missing base-axis dimensions should not collapse the rate to zero, and filtered data whose extent already lies inside the axis window does not need another full count pass. This keeps none/empty mode on the visible-window scan path, while allowing filter mode to use the already filtered data count when the extent proves it is safe. Constraint: dataZoom filterMode none/empty keep raw data count even when the axis window is narrow Rejected: Use count whenever count differs from raw count | unsafe when the current data extent is not fully inside the base-axis scale Confidence: high Scope-risk: narrow Directive: Keep countDataInAxisExtent as the fallback for unfiltered and extent-ambiguous dataZoom modes Tested: npm run checktype -- --pretty false; npm run lint -- --quiet src/processor/dataSample.ts; git diff --check -- src/processor/dataSample.ts; Chrome headless HTML regression screenshot Not-tested: Full visual regression suite
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot review pointed out that the previous fast path still scanned when the processed data count matched the raw count, even if the base-axis scale already contained the data extent. The extent check alone is enough to decide whether a visible-window count is needed. This preserves the scan for dataZoom none/empty narrow windows, while avoiding it for the fully visible or already-filtered cases whose data extent is inside the current axis extent. Constraint: dataZoom none/empty can keep raw count while the visible axis window is narrow Rejected: Keep raw-count equality as a scan trigger | it regresses the fully visible common path Confidence: high Scope-risk: narrow Directive: Use the base-axis extent relationship, not raw-count equality, to decide whether the count scan is necessary Tested: git diff --check -- src/processor/dataSample.ts; npm run lint -- --quiet src/processor/dataSample.ts; npm run checktype -- --pretty false; Chrome headless HTML regression screenshot Not-tested: Full visual regression suite
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot review pointed out that dataZoom empty mode writes the zoom window into processed data approximate extents. Using that processed extent to skip the visible-count scan can therefore keep the sampling rate tied to the full dataset. Use processed extents only when the data count proves the series was actually filtered. For none, empty, and fully visible data, use raw data extents to decide whether the count scan is necessary. Constraint: dataZoom filterMode empty preserves item count while replacing out-of-window axis values with NaN Rejected: Always trust processed approximate extent | empty mode intentionally overwrites it with the zoom window Confidence: high Scope-risk: narrow Directive: Do not use processed approximate extent as a fully-visible shortcut when count still matches raw count Tested: ./node_modules/.bin/eslint --quiet src/processor/dataSample.ts; ./node_modules/.bin/tsc --noEmit --pretty false; git diff --check -- src/processor/dataSample.ts; Chrome headless HTML regression screenshot Not-tested: Full visual regression suite
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Fixes #21512 by making line sampling calculate its sampling rate from the current base-axis visible extent instead of only the post-filter
SeriesDatacount.With
dataZoom.filterMode: 'none'or'empty', ECharts intentionally keeps the full data list. The old sampling code therefore kept using the full dataset size after zooming in, so narrow windows could remain downsampled and lose detail. This change keepsfilterbehavior intact while allowingnoneandemptyto reveal raw detail when the visible window is small enough.Changes
sampling: 'none'to avoid an unnecessary pass over default no-op sampling.filter,none + lttb,empty + lttb, andnone + no samplingaround a narrow zoom window.Verification
npm run checktype -- --pretty falsenpm run lint -- --quiet src/processor/dataSample.tsgit diff --check -- src/processor/dataSample.ts test/line-sampling-dataZoom-filterMode.htmltest/line-sampling-dataZoom-filterMode.htmlin Chrome with the local fixed bundle: PASS;noneandemptyprocessed counts match raw data count in the zoomed window scenario.