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

Improve tick generation for linear scales #5938

Merged
merged 3 commits into from Jan 1, 2019
Merged

Conversation

nagix
Copy link
Contributor

@nagix nagix commented Dec 26, 2018

There are a few problems in tick generation for linear scales:

  1. When maxTicksLimit is not set, the number of ticks can exceed 11 which is the default value of maxTicksLimit (see a and b in the example below).
  2. autoSkip: properly calculate space needed by tick label #5922 fixed an issue in _autoSkip so that the number of ticks stay within maxTicksLimit, however tick values are not aligned to a nice number and the tick at the min value is missing (see c and d).
  3. When both stepSize and maxTicksLimit are set, maxTicksLimit doesn’t limit the number of ticks as reported in Using stepSize and maxTicksLimit together doesn't work. #2539. In this case, stepSize should work as the minimum unit of stepSize for ticks, and it should be multiplied by a nice number (see d).
  4. Auto-generated ticks differ depending on maxTicksLimit despite it doesn’t actually limit the number of ticks (see e and f).
  5. Ticks in a radar chart overlaps each other as mentioned in Add scale.pointLabels.lineHeight and scale.ticks.lineHeight options #5914. autoSkip: properly calculate space needed by tick label #5922 didn’t fix this as the maximum number of ticks based on the scale size is not used in _autoSkip but in generateTicks (see g).

Version 2.7.3: https://jsfiddle.net/nagix/eygnmo5b/

screen shot 2018-12-27 at 1 28 11 am

Master: https://jsfiddle.net/nagix/jds1c98z/

screen shot 2018-12-27 at 1 28 24 am

This PR improves tick generation for linear scales by adding the following logic:

  1. In the getTickLimit method, maxTicks is calculated based not only on maxTicksLimit, the default limit (11), the default spacing (40 pixels) or the tick font size but also on stepSize.
  2. In the generateTicks function,
    1. Find a nice number of the dataRange divided by maxNumSpaces for spacing. stepSize is used as a minimum unit if it is specified.
    2. Find the number of spaces (numSpaces) such that both dataRange.min and dataRange.max are contained in the range.
    3. If numSpaces exceeds maxNumSpaces, spacing is set to a nice number of numSpaces * spacing / maxNumSpaces, which guarantees that dataRange.min and dataRange.max are contained in the range. stepSize is used as a minimum unit if it is specified.
    4. niceMin and niceMax are adjusted separately based on min, max and stepSize.

Master + This PR: https://jsfiddle.net/nagix/7jxefc94/

screen shot 2018-12-27 at 1 28 39 am

Fixes #1968
Fixes #2539

@benmccann
Copy link
Contributor

I was confused by the chart for c with this PR because the first bar was not displaying at all. I realized after looking at the data that it's because the value is 0.5 and the axis starts at 0.5. Can we have the axis start at 0? That would solve the problem of the disappearing bar. It's also much clearer as to the relative values. It's frequently called out as misleading to not have axis start at 0:
https://callingbullshit.org/tools/tools_misleading_axes.html
https://flowingdata.com/2015/08/31/bar-chart-baselines-start-at-zero/

@simonbrunel
Copy link
Member

@benmccann Can we have the axis start at 0?

No (unfortunately). While I agree that scales should start at 0 by default (still need to be able to change that behavior to satisfy all use cases), it's currently not a bug but a feature (scale.beginAtZero: boolean (default false)) so changing this would break many projects. Though, I don't think that topic belongs to that PR.

Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

@nagix I like the new behavior, however I think the code is getting too complex. Could we try to refactor it (per my review comments) and apply the same "max ticks" logic whatever the stepSize value?

src/scales/scale.linearbase.js Outdated Show resolved Hide resolved
src/scales/scale.linearbase.js Outdated Show resolved Hide resolved
src/scales/scale.linear.js Outdated Show resolved Hide resolved
src/scales/scale.linear.js Outdated Show resolved Hide resolved
@nagix
Copy link
Contributor Author

nagix commented Dec 28, 2018

I have simplified the code and updated the description above.

@kurkle
Copy link
Member

kurkle commented Dec 28, 2018

Rounding could be smarter (works the same in all 3 versions):

image

Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

Looks much cleaner than original now. Functionality seems good.
Only almost equal getTickLimit functions bother me.

src/scales/scale.linear.js Outdated Show resolved Hide resolved
@simonbrunel simonbrunel added this to the Version 2.8 milestone Dec 29, 2018
Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

LGTM, just a minor change.

@@ -16,35 +16,41 @@ function generateTicks(generationOptions, dataRange) {
// for details.

var stepSize = generationOptions.stepSize;
Copy link
Member

Choose a reason for hiding this comment

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

stepSize < 0 doesn't seems a valid value, so maybe we can simplify all these checks by clamping this value:

var stepSize = Math.max(0, generationOptions.stepSize);
var unit = stepSize || 1;
//...

src/scales/scale.linearbase.js Outdated Show resolved Hide resolved
src/scales/scale.linearbase.js Outdated Show resolved Hide resolved
@nagix
Copy link
Contributor Author

nagix commented Dec 29, 2018

Rounding could be smarter (works the same in all 3 versions):

I have been aware of this, which is the separate issue in the tick formatter. Actually, generateTicks generates accurate values ([2.5, 2.25, 2.0, ...]) but Chart.Ticks.formatters.linear rounds values.

@nagix
Copy link
Contributor Author

nagix commented Dec 29, 2018

@kurkle @simonbrunel Thanks for your comments. I refactored getTickLimit and added minor modification.

exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
* Improve tick generation for linear scales
* Simplify the tick generation code
* Refactor getTickLimit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using stepSize and maxTicksLimit together doesn't work. v2.0 max ticks labeling issue
5 participants