Skip to content

fix: bar smapling not work bug. fix#4571#4573

Open
skie1997 wants to merge 1 commit intodevelopfrom
fix/bug-for-sampling
Open

fix: bar smapling not work bug. fix#4571#4573
skie1997 wants to merge 1 commit intodevelopfrom
fix/bug-for-sampling

Conversation

@skie1997
Copy link
Copy Markdown
Contributor

[中文版模板 / Chinese template]

🤔 This is a ...

  • New feature
  • Bug fix
  • TypeScript definition update
  • Bundle size optimization
  • Performance optimization
  • Enhancement feature
  • Refactoring
  • Update dependency
  • Code style optimization
  • Test Case
  • Branch merge
  • Release
  • Site / documentation update
  • Demo update
  • Workflow
  • Other (about what?)

🔗 Related issue link

close #4571

🔗 Related PR link

🐞 Bugserver case id

💡 Background and solution

📝 Changelog

Language Changelog
🇺🇸 English
🇨🇳 Chinese

☑️ Self-Check before Merge

⚠️ Please check all items below before requesting a reviewing. ⚠️

  • Doc is updated/provided or not needed
  • Demo is updated/provided or not needed
  • TypeScript definition is updated/provided or not needed
  • Changelog is provided or not needed

🚀 Summary

copilot:summary

🔍 Walkthrough

copilot:walkthrough

@xuefei1313
Copy link
Copy Markdown
Contributor

🦞 Aime Bot Review

改动摘要:本 PR 针对 bar 系列在开启 sampling 时不生效的问题,将数据变换流水线中的 transform 类型从 sampling 调整为 dataSampling,并新增了一条带 sampling: 'lttb' 的 bar 单元测试和对应的 changelog 记录,用来验证修复效果。

代码层面的具体观察:在 BarSeries 中,对 _data 的 transform 配置改为 type: 'dataSampling',仍然沿用原有的 size / factor / yfield 参数,看起来是与数据集内部对采样 transform 的命名保持一致,从而让 sampling 配置真正生效。建议作者自查一下其他系列(例如 line/area)中是否也是使用 dataSampling 这个 transform 名,以确认命名统一,避免后续出现类似问题。新增的单元测试通过构造 5 条简单的 bar 数据并开启 sampling: 'lttb',在完整的 chart 生命周期后断言 _data.getProduct() 长度仍为 5,覆盖了“采样不应在 bar 上误删数据”的主路径。后续如果有堆叠/水平 bar 等更多组合场景,可以按需再补充测试用例,属于增强建议,不阻塞当前修复。

小提示:PR 标题以及 changelog 中的文案里 smapling 建议改成 sampling,这个属于小拼写问题,不影响功能,可以后续有机会顺带修正。

合并建议:整体改动范围很小、意图明确,且有针对性的单测覆盖。如果已经在本地验证过其他典型场景(例如不同方向或不同 samplingFactor),个人认为这个修复是 OK 的,可以作为 bugfix 合入。

@Issues-translate-bot
Copy link
Copy Markdown

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


🦞Aime Bot Review

Summary of changes: This PR aims at the problem that the bar series does not take effect when sampling is turned on. The transform type in the data transformation pipeline is adjusted from sampling to dataSampling, and a new bar unit test with sampling: 'lttb' and the corresponding changelog record are added to verify the repair effect.

Specific observations at the code level: In BarSeries, the transform configuration of _data is changed to type: 'dataSampling', and the original size / factor / yfield parameters are still used. It seems to be consistent with the naming of the sampling transform inside the data set, so that the sampling configuration can truly take effect. It is recommended that the author check whether the transform name dataSampling is also used in other series (such as line/area) to confirm that the naming is unified and avoid similar problems in the future. The new unit test constructs 5 simple bar data and turns on sampling: 'lttb', and asserts that the length of _data.getProduct() is still 5 after the complete chart life cycle, covering the main path of "sampling should not accidentally delete data on bar". If there are more combination scenarios such as stacking/horizontal bars in the future, you can add test cases as needed. This is an enhancement suggestion and does not block the current fix.

Tip: It is recommended to change smapling to sampling in the PR title and the copy in the changelog. This is a minor spelling problem and does not affect the functionality. It can be corrected later.

Merger suggestion: The overall scope of changes is small, the intention is clear, and targeted single test coverage. If other typical scenarios have been verified locally (such as different directions or different samplingFactor), I personally think this fix is ​​OK and can be incorporated as a bugfix.

@xuefei1313
Copy link
Copy Markdown
Contributor

@codex review

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.

[Bug] 柱状图配置sampling之后,不显示

3 participants