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

Setting groupPixelWidth to 0 in plotOptions prevents forced data grouping in rangeSelector #20913

Open
cebamps opened this issue Mar 22, 2024 · 5 comments

Comments

@cebamps
Copy link

cebamps commented Mar 22, 2024

Expected behaviour

The behaviour documented in rangeSelector.buttons.dataGrouping.forced should remain even when plotOptions.line.dataGrouping.groupPixelWidth is set to 0.

When data grouping is forced, it runs no matter how small the intervals are. This can be handy for example when the sum should be calculated for values appearing at random times within each hour.

Actual behaviour

When I set plotOptions.line.dataGrouping.groupPixelWidth to zero, my range selector buttons no longer force data grouping.

As a workaround, a small non-zero groupPixelWidth in plotOptions works. But setting it to a very small value is not a good idea, because it is used somewhere to determine tick density in an inversely proportional way.

Another workaround is to reset groupPixelWidth to its default value of 2 in my rangeSelector.buttons.dataGrouping options.

Live demo with steps to reproduce

https://jsfiddle.net/cedricbaptk/jx81mopf/

  • According to the configuration, the chart should initially display with data aggregated by month. This is not the case, which demonstrates the issue.
  • Reloading after commenting out the groupPixelWidth: 0 plotOptions configuration demonstrates the expected behaviour.

Product version

Highstock 11.4.0 (latest).
Highstock 10.3.2.

Affected browser(s)

n/a

@karolkolodziej
Copy link
Contributor

Hi @cebamps!

It is because the groupPixelWidth: 0 acts as a falsy value here and prevents us from entering the if statement.

// Execute grouping if the amount of points is greater than the limit
// defined in groupPixelWidth
if (
groupPixelWidth &&
processedXData &&
processedXData.length &&
plotSizeX
) {

I agree that when the forced flag is set, the groupPixelWidth shouldn't matter that much
But in other cases, value 0 for it doesn't make sense.

@pawelfus
Maybe the getGroupPixelWidth might be improved and if the forced is set it should take the default groupPixelWidth?

@pawelfus
Copy link
Contributor

Hello @cebamps

Thanks for reporting the issue! You said:

When I set plotOptions.line.dataGrouping.groupPixelWidth to zero, my range selector buttons no longer force data grouping.

Why do you want to set groupPixelWidth to zero pixels? This option is designed to determine when dataGrouping should start grouping. If you set force = true, then the option is unnecessary.

As a workaround, a small non-zero groupPixelWidth in plotOptions works. But setting it to a very small value is not a good idea, because it is used somewhere to determine tick density in an inversely proportional way.

It is not about ticks, but points' positions. I am certain, you don't want to render points 0px away from each other.

@cebamps
Copy link
Author

cebamps commented Mar 28, 2024

Hello @pawelfus, thanks for your response.

Why do you want to set groupPixelWidth to zero pixels? This option is designed to determine when dataGrouping should start grouping. If you set force = true, then the option is unnecessary.

My intent is to disable data grouping unless forced through range selector buttons. One representative example is a responsive chart plotting an integer value over time. Since the point density depends on what data query was performed and how narrow my responsive chart gets, there were some charts where grouping kicked in and started displaying averages where I really wanted to present integer values.

I could go for a different aggregation method like computing a median, but in this scenario I would rather suffer the possible performance impact and display the actual data. This is where I expected groupPixelWidth = 0 to help me.

It is not about ticks, but points' positions. I am certain, you don't want to render points 0px away from each other.

Yes, definitely. I just meant to highlight my first bad attempt as a workaround (if zero doesn't work, try a very small positive value), and not to make it too much of a point of focus in this issue report, because I am no longer trying to make that workaround work for me.

Just to expand a little more on why this workaround did not work for me though, in case someone else is tempted by the same approach... The workaround does work to disable grouping, but it has an unfortunate consequence in the chart configuration I used, causing the browser to try and fail to allocate a huge array. I guess this might be a separate bug?

Here is a demo fiddle. Clicking the button in Chrome (version 123.0.6312.59) causes this error to appear in the console after a bit (Firefox instead hangs for a few seconds then logs "Uncaught out of memory"):

highstock.src.js:4151 Uncaught RangeError: Invalid array length
at Array.push (<anonymous>)
at Time.getTimeTicks (highstock.src.js:4151:39)
at Axis.getTimeTicks (highstock.src.js:49620:53)
at LineSeries.applyGrouping (highstock.src.js:56871:65)
at highstock.src.js:56591:24
at Array.forEach (<anonymous>)
at Axis.applyGrouping (highstock.src.js:56583:20)
at highstock.src.js:1779:32
at Array.forEach (<anonymous>)
at fireEvent (highstock.src.js:1776:24)

After writing all this, I do realise that my approach to configuration may be a little too naive. For my use case, I think it would make sense for me to look into custom data grouping approximations to implement a form of decimation on the chart data, so that I show true data points without having to render all of them.

@pawelfus
Copy link
Contributor

Thank you for the details @cebamps !

My intent is to disable data grouping unless forced through range selector buttons.

Did you try using dataGrouping.enabled option? Something like this: https://jsfiddle.net/BlackLabel/wdoacb2z/3/

With responsive options, you should get quite good results. However, you might be right - it might be easier to maintain everything in approximation function.

Alternatively, you can use chart.xAxis[0].setDataGrouping(DataGroupingOptions | false, [redraw=true]) method, to set DataGrouping to all series connected to one xAxis. For example chart.xAxis[0].setDataGrouping(false); disables DataGrouping.

Just to expand a little more on why this workaround did not work for me though, in case someone else is tempted by the same approach... The workaround does work to disable grouping, but it has an unfortunate consequence in the chart configuration I used, causing the browser to try and fail to allocate a huge array. I guess this might be a separate bug?

With this config, we try to generate a group for every.. 0.000001ms. With the data range (63158540000ms) it gives 63158540000000000 groups. That value is higher than the max safe integer in JS (9007199254740991), so we can't even iterate properly over this array 😅

@cebamps
Copy link
Author

cebamps commented Mar 29, 2024

Hi @pawelfus, thanks for the suggestion. That does indeed make more sense, and working over dataGrouping.enabled to enable data grouping obviously feels much more straightforward indeed. I will try and change my approach to configuration 👍

With this config, we try to generate a group for every.. 0.000001ms. With the data range (63158540000ms) it gives 63158540000000000 groups. That value is higher than the max safe integer in JS (9007199254740991), so we can't even iterate properly over this array 😅

Right, over time my mental model for groupPixelWidth had grown into a threshold rather than the actual width of groups. When you put it the way you did, setting groupPixelWidth to a small value does indeed feel wrong 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants