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

Bugged rounding algorithm when setting Extremes for y axis #17100

Closed
ThomasK0lasa opened this issue Mar 17, 2022 · 6 comments · Fixed by #17110
Closed

Bugged rounding algorithm when setting Extremes for y axis #17100

ThomasK0lasa opened this issue Mar 17, 2022 · 6 comments · Fixed by #17110

Comments

@ThomasK0lasa
Copy link

ThomasK0lasa commented Mar 17, 2022

Expected behaviour

Better rounding: 106 rounded to 120;

Actual behaviour

Wrong rounding 106 rounded to 200. This happens for ONLY specific min and max. Take a look at examples below.

Live demo with steps to reproduce

Proper rounding: 99 rounded to 100 > https://jsfiddle.net/y9cz1Lu0/4/
Proper rounding: 112 rounded to 120 > https://jsfiddle.net/y9cz1Lu0/5/
WRONG rounding: 106 rounded to 200 (BUGGED) > https://jsfiddle.net/2mn8yd0k/2/

As You can see the rounding for specific min max is wrongly calculated (106 rounded to 200?). Please don't tell me that I can do endOnTick: false or change the tickAmount as this is not solving the problem, but merely omitting it.

Product version

Highcharts JS v10.0.0 (2022-03-07)

Affected browser(s)

Chrome 99.0.4844.51

@karolkolodziej
Copy link
Contributor

In steps how this mechanism works for alleged bugged demo:

  1. In the setTickInterval method we calculate tickInterval = 25
  2. Then in the setTickPositions we are trying to calculate where on the axis ticks should be.
    This results in tickPositions = [0, 25, 50, 75, 100, 125]
  3. As you can see, the amount of ticks is greater than what you set by tickAmount: 5 that is why the adjustTickAmount has to be invoked.
  4. There the tick interval is incremented like this axis.tickInterval *= 2 and setTickPositions fired once again.
  5. This time we get tickPositions = [0, 50, 100, 150].
  6. Now tick amount is smaller than the one set in the options.
    That is why the algorithm adds another tick and we get [0, 50, 100, 150, 200]

This mechanism is sensitive to the data and axis options and results might differ in case of a small change.
@TorsteinHonsi do you see any room for improvement?

@TorsteinHonsi
Copy link
Collaborator

Thanks for debugging @karolkolodziej! Yes it is true that this approach is sensitive to small variations in the data extremes. It seems like this is an unfortunate edge case where neither step 2 nor step 5 hit the mark, so we eventually end up with too much padding.

The only thing I can think of if we were able to modify those steps to hit the desired tickAmount right away, so we didn't have to modify it later. Feel free to experiment with it.

@TorsteinHonsi
Copy link
Collaborator

I'm trying a more granular increment instead of the *2 in step 4. Pushing a POC soon, let's see what visual differences the CI finds.

@karolkolodziej
Copy link
Contributor

@ThomasK0lasa your chart with the new algorithm: https://jsfiddle.net/BlackLabel/vxn7j9L4/

@ThomasK0lasa
Copy link
Author

@karolkolodziej thank you. Looks great. When this fix will be available in production? More or less.

@karolkolodziej
Copy link
Contributor

The next release is planned for the following month.

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

Successfully merging a pull request may close this issue.

3 participants