Skip to content

Conversation

JCQuintas
Copy link
Member

Fixes #19067

@JCQuintas JCQuintas self-assigned this Aug 5, 2025
@JCQuintas JCQuintas added type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature. scope: charts Changes related to the charts. labels Aug 5, 2025
@mui-bot
Copy link

mui-bot commented Aug 5, 2025

Deploy preview: https://deploy-preview-19069--material-ui-x.netlify.app/

Bundle size report

Bundle Parsed Size Gzip Size
@mui/x-data-grid 0B(0.00%) 0B(0.00%)
@mui/x-data-grid-pro 0B(0.00%) 0B(0.00%)
@mui/x-data-grid-premium 0B(0.00%) 0B(0.00%)
@mui/x-charts ▼-221B(-0.07%) ▼-72B(-0.08%)
@mui/x-charts-pro ▼-221B(-0.06%) ▼-58B(-0.05%)
@mui/x-date-pickers 0B(0.00%) 0B(0.00%)
@mui/x-date-pickers-pro 0B(0.00%) 0B(0.00%)
@mui/x-tree-view 0B(0.00%) 0B(0.00%)
@mui/x-tree-view-pro 0B(0.00%) 0B(0.00%)

Details of bundle changes

Generated by 🚫 dangerJS against 9fe286d

Copy link

codspeed-hq bot commented Aug 5, 2025

CodSpeed Performance Report

Merging #19069 will improve performances by 9.12%

Comparing JCQuintas:grouped-logic (9fe286d) with master (e61c067)1

Summary

⚡ 1 improvements
✅ 9 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
ScatterChartPro with big data amount 437.7 ms 401.1 ms +9.12%

Footnotes

  1. No successful run was found on master (d726c55) during the generation of this report, so e61c067 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

labelOffset,
});

if (!storedOffsets.has(tickOffset)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be feasible to rely on the dataIndex to avoid using floating point number as a key?

This part of the code is quite hard to get. Especially because storedOffsets is not very explicit. Whay those offset are stored?

Maybe something like offsetsToTickIndexes/dataIndexToTickIndes with a comment

// Map the axis dataIndexes to the tick indexes related.
const dataIndexToTicks = new Map<number, Set<number>>();

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't use dataIndex in this case, we need to know exactly where the tick is, as this is used to remove duplicate ticks.

Floats should be fairly consistent overall, they are just not precise. Can you see an issue that might be created by this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you see an issue that might be created by this?

I don't get why we need the position and not the dataIndex.
The tickOffset is computed from the scale, the tickValue=tickValues[dataIndex], and tickPlacement

Either tickPlacement is the same for all groups. In that case, same tickOffset is equivalement to same dataIndex. Or tickPlacement can change from one group to another. But in that case it seems risky to rely on the fact that tickPlacement='start' of an item and tickPlacement='end' of the previous item get exactly the same floating number

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah true, because the ticks are always in the same place 🤦
I'll change it

Or tickPlacement can change from one group to another. But in that case it seems risky to rely on the fact that tickPlacement='start' of an item and tickPlacement='end' of the previous item get exactly the same floating number

As long as the original numbers are the same the results should be the same.

@JCQuintas JCQuintas enabled auto-merge (squash) August 7, 2025 10:02
@JCQuintas JCQuintas merged commit cdea241 into mui:master Aug 7, 2025
23 checks passed
@JCQuintas JCQuintas deleted the grouped-logic branch August 7, 2025 10:23

// Calculate the label offset
const labelOffset =
(scale as { step: () => number }).step() *
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we update the scale type to be D3DiscreteScale (and create that type) instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: charts Changes related to the charts. type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[charts] Improve axis grouping logic
4 participants