fix(sample): fix minmax sampling behaviour

[ribeirompl]

Brief Information

This pull request is in the type of:

• bug fixing
• new feature
• others

What does this PR do?

This fixes the minmax sampling behaviour to follow the standard as described here.

Fixed issues

No open issue, but the problem was discussed in the original PR for this feature: #19279 (comment)

Details

Before: What was the problem?

I generated some fake data, 16000 data points for each graph (green random walk, yellow sine wave, and blue pseudo-random).
I added two artificial "spikes" for each graph: a downward spike at index 7000, and an upward spike at index 9000.

No downsampling	Original minmax downsampling implementation

The original minmax implementation appears to act just like the "max" downsampling method for data points that are above y=0, and thereby it misses out on any downward spikes.

After: How does it behave after the fixing?

No downsampling	New minmax downsampling implementation (2pts per px)

The new implementation more closely follows the actual trend, and ensures that it captures all up and down spikes.

Brief example of the minmax downsampling method, as described here:

1. Example data: [4, 1, 7, 8, 3, 6, 5, 9, 2, 7, 8, 1]
2. Frame size of 3: [(4, 1, 7), (8, 3, 6), (5, 9, 2), (7, 8, 1)]
3. Identify the min and max within each frame: [(4, 1, 7), (8, 3, 6), (5, 9, 2), (7, 8, 1)]
4. Keep the order of the min and max within each group, and those become the resulting downsampled set: [1, 7, 8, 3, 9, 2, 8, 1]

Discussion Point A: the link above suggests using two frames for each pixel, whereby each frame results in two data points, for a total density of 4 data points per pixel. Personally I think this is a bit overkill, as it results in less performance for minimal graph quality increase.
The current implementation in this PR has 2 data points per pixel, however this could be halved to 1 data point if you agree. The benefit of 1 data point per pixel is that the graph is more likely to allow graph animations, whereas the higher density may be above the threshold for animations still to play.
See comparison below:

No downsampling	New minmax downsampling implementation (2pts per px)

As suggested by link (4pts per px)	Doubled rate for 1pt per px

I suggest opening the images in fullscreen for a better comparison.

Discussion Point B: Regarding the code, I referenced the lttb method for implementing this minmax method in a similar way (i.e. having a separate function for downsampling), since the regular downsampling method would not work due to the minmax method requiring the ability to output two samples per frame. It may be needed to add an extra condition to the downsample method selection logic, to check if the rate is high enough (>2) to warrant downsampling, since this method isn't as efficient as the others. However, this will depend on the decision for Discussion Point A above.

Document Info

One of the following should be checked.

• This PR doesn't relate to document changes
• The document should be updated later
• The document changes have been made in apache/echarts-doc#xxx

Misc

ZRender Changes

• This PR depends on ZRender changes (ecomfe/zrender#xxx).

Related test cases or examples to use the new APIs

N.A.

Others

Other information

[echarts-bot]

Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout the coding standard and Wiki about How to make a pull request.

[Ovilia]

@snukhulov Can you help review this PR? Thanks!

[snukhulov]

@snukhulov Can you help review this PR? Thanks!

The proposed minmax algorithm is more powerful than the previous one, it resolves the discussed problems

[echarts-bot]

Congratulations! Your PR has been merged. Thanks for your contribution! 👍