Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[charts-pro] Zoom axis filtering #14121

Merged
merged 51 commits into from
Aug 20, 2024
Merged

Conversation

JCQuintas
Copy link
Member

@JCQuintas JCQuintas commented Aug 6, 2024

Resolves #12083, #13973

ECharts filter behaviour: line, bar, scatter

Changelog

@JCQuintas JCQuintas added new feature New feature or request component: charts This is the name of the generic UI component, not the React module! labels Aug 6, 2024
@JCQuintas JCQuintas self-assigned this Aug 6, 2024
@mui-bot
Copy link

mui-bot commented Aug 6, 2024

Copy link

codspeed-hq bot commented Aug 6, 2024

CodSpeed Performance Report

Merging #14121 will not alter performance

Comparing JCQuintas:zoom-axis-filtering (e35da13) with master (a593729)

Summary

✅ 3 untouched benchmarks

JCQuintas and others added 5 commits August 6, 2024 14:39
Co-authored-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>
Signed-off-by: Jose C Quintas Jr <juniorquintas@gmail.com>
@alexfauquette
Copy link
Member

Sorry for the force push. I forgot to create a new branch to discuss code variation. Here is the proposal I miss published #14131

From your last commits, I assume we are not considering the same feature.

You are removing points based on the zoom state. But for me the feature was the opposite: defining the zoom state (when not controlled by the user) based on points to render.

Here is a Echart example

echarts.mp4

Here is a demo of the proposed PR

barchart.mp4

Could you details what you would like to acceive by removing those data points from the rendering? I fear it creates bad artefects

image

@JCQuintas
Copy link
Member Author

JCQuintas commented Aug 7, 2024

@alexfauquette no worries, I can deal with force pushes 😆

I think in the end our target is both my implementation and yours. I think some of these behaviours are indeed a bit odd, but I'm copying from echarts. Filtering behaves differently on X vs Y axis. You can see on the examples below.

line, bar, scatter

For line charts you can see the behaviours

Mode: empty for both X and Y
Behaviour:
Y Axis: empty
X Axis: filter

Screen.Recording.2024-08-07.at.11.58.32.mov

Mode: filter for both X and Y
Behaviour: filter for both X and Y

Screen.Recording.2024-08-07.at.11.58.07.mov

@alexfauquette
Copy link
Member

Axis interaction

Goal

For me, the main goal is that when zooming on the x-axis, then the y-axis can zoom in/out to show the relevant part of the chart.

For example in the next drawing, if the x-axis is set to filter, the two out-sider from the left and the right are ignored when computing the y-axis extremums, which makes the chart zoom on the data point along the y-axis

image

Code perspective

Here is a basic representation of the data pipeline. With green dot where you are implementing filtering logic (the computeValue function, and the LinePLot)

I don't see how in the computeValue for y-axis you could know that some data are to be ignored, because the x-axis filtered them out. And the same in the opposite direction: x-axis being aware of which data are filtered by the y-axis.

For me the only point where this can be done is before doing the per axis computation (the blue spot)

image

Filtering on plot

About how Echarts display data, I would not copy exactly their behavior. As a user I would be annoyed to have those data points removed from the display, and it seems I'm not the only one 😇 apache/echarts#3637

For me the way axis zoom can impact other axes is something to keep. It's what users asked in #12083
But filtering data in a way that impacts the rendered graphics is to avoid

@JCQuintas
Copy link
Member Author

Gotcha, so I was reaching for the more complex goal which isn't necessary 😅

Would you say the goal then is:

When zooming on the main axis, update the secondary axis(1+) so as to display only the information relevant to the data currently displayed in the viewport.

?

@alexfauquette
Copy link
Member

alexfauquette commented Aug 7, 2024

Would you say the goal then is:

When zooming on the main axis, update the secondary axis(1+) so as to display only the information relevant to the data currently displayed in the viewport.

Yes, and this filterMode is a way to define which axis is the main one.

  • filterMode: 'filter': you're the main axis, you have ability to remove data to other ones
  • filterMode: 'keep': you're the secondary axis, your zoom does not impact others

And that seems to be all we need.

What changes

Before the extremum getters were looping other series to find their max/min values along one axis. Now they need to do that while taking care that the second axis is not filtering out those data points.

@JCQuintas JCQuintas changed the title [charts] Zoom axis filtering [charts-pro] Zoom axis filtering Aug 8, 2024
@alexfauquette
Copy link
Member

Does it work on other scales? 🤔

No, it's not the scale but the axis.data that causes the issue. You can not filter along an axis if its data are defined through the dataset

@JCQuintas
Copy link
Member Author

@alexfauquette should be fixed. Moving the dataset binding to the useDefaultizeAxis should solve the issue

Copy link
Member

@alexfauquette alexfauquette left a comment

Choose a reason for hiding this comment

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

Left a last renaming point to fix

That's a very nice feature. Could benefit a changelog highlight

You can do so by adding ## Changelog to the PR description. The following content will be copy/pasted by the changelog generation script, such that we do not forget it

) => {
const xAxis = React.useMemo(() => defaultizeAxis(inXAxis, 'x'), [inXAxis]);
const yAxis = React.useMemo(() => defaultizeAxis(inYAxis, 'y'), [inYAxis]);
const xAxis = React.useMemo(() => defaultizeAxis(inXAxis, dataset, 'x'), [inXAxis, dataset]);
Copy link
Member

Choose a reason for hiding this comment

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

For me this would be clearer but maybe it causes TS issues. DOes not really matter

Suggested change
const xAxis = React.useMemo(() => defaultizeAxis(inXAxis, dataset, 'x'), [inXAxis, dataset]);
const xAxis = React.useMemo(() => normalize(defaultizeAxis(inXAxis, 'x'), dataset, 'x'), [inXAxis, dataset]);

Copy link
Member

@alexfauquette alexfauquette left a comment

Choose a reason for hiding this comment

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

Left a last renaming point to fix

That's a very nice feature. Could benefit a changelog highlight

You can do so by adding ## Changelog to the PR description. The following content will be copy/pasted by the changelog generation script, such that we do not forget it

JCQuintas and others added 2 commits August 20, 2024 11:03
Co-authored-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>
Signed-off-by: Jose C Quintas Jr <juniorquintas@gmail.com>
@JCQuintas JCQuintas enabled auto-merge (squash) August 20, 2024 09:35
@JCQuintas JCQuintas merged commit ea3fbcc into mui:master Aug 20, 2024
17 checks passed
@JCQuintas JCQuintas deleted the zoom-axis-filtering branch August 20, 2024 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: charts This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[charts] Axis min/max does not allow dynamic rescaling of the other Axis when that axis has no min/max set
3 participants