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

Add ticks.integerSteps option to linear scale. #4841

Merged
merged 4 commits into from
Apr 1, 2018
Merged

Add ticks.integerSteps option to linear scale. #4841

merged 4 commits into from
Apr 1, 2018

Conversation

etimberg
Copy link
Member

When true, and stepSize, is not specified, the generated step size will be increased
to the nearest integer value.

Resolves #4103

@benmccann
Copy link
Contributor

My main question with this PR is how do we want tick generation and the autoskipper to interact? E.g. the time scale has a lot of tick generation logic, but I think @simonbrunel would prefer that we remove most of it and rely on the autoskipper instead. I'm wondering if we want this logic in the autoSkipper in that case? E.g. an option like skipNonIntegerTicks or something along those lines

@etimberg
Copy link
Member Author

etimberg commented Nov 1, 2017

That's a good question. I haven't really thought about the auto skipper. In an imagined redesigned auto skipper, having extra options would be nice. But reducing the overall number of ticks generated does have other implications like the fact that the auto skipper would have less things to consider which may make it faster

@etimberg
Copy link
Member Author

@simonbrunel @benmccann I rebased this so it should pass the CI now

@simonbrunel
Copy link
Member

@etimberg what do you think about a more generic option that allows to choose the rounding step, for example precision, a bit like jasmine toBeCloseTo:

  • if undefined or false, the input is unchanged (eq. integerSteps: undefined/false)
  • if 0, the input is rounded to the upper value (eq. integerSteps: true)
  • if > 1, The number of decimal points to keep

@etimberg
Copy link
Member Author

Sure, that works for me

`stepSize`, is not specified, the generated step size will be increased
to the nearest integer value.
if (generationOptions.stepSize && generationOptions.stepSize > 0) {
spacing = generationOptions.stepSize;
} else {
var niceRange = helpers.niceNum(dataRange.max - dataRange.min, false);
spacing = helpers.niceNum(niceRange / (generationOptions.maxTicks - 1), true);

var spacingPrecision = generationOptions.precision;
Copy link
Contributor

Choose a reason for hiding this comment

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

@etimberg I think it would be better rename the previous precision variable to factor and reuse it, instead of introducing the variable spacePrecision. This to improve readability and as it serves the same purpose later in the function.

function generateTicks(generationOptions, dataRange) {
	var ticks = [];
	// To get a "nice" value for the tick spacing, we will use the appropriately named
	// "nice number" algorithm. See http://stackoverflow.com/questions/8506881/nice-label-algorithm-for-charts-with-minimum-ticks
	// for details.

	var factor = 1;
	var spacing;

	if (generationOptions.stepSize && generationOptions.stepSize > 0) {
		spacing = generationOptions.stepSize;
	} else {
		var niceRange = helpers.niceNum(dataRange.max - dataRange.min, false);
		spacing = helpers.niceNum(niceRange / (generationOptions.maxTicks - 1), true);
		if (generationOptions.precision !== undefined) {
			factor = Math.pow(10, generationOptions.precision);
			spacing = Math.ceil(spacing * factor) / factor;
		}
	}
	var niceMin = Math.floor(dataRange.min / spacing) * spacing;
	var niceMax = Math.ceil(dataRange.max / spacing) * spacing;

	// If min, max and stepSize is set and they make an evenly spaced scale use it.
	if (generationOptions.min && generationOptions.max && generationOptions.stepSize) {
		// If very close to our whole number, use it.
		if (helpers.almostWhole((generationOptions.max - generationOptions.min) / generationOptions.stepSize, spacing / 1000)) {
			niceMin = generationOptions.min;
			niceMax = generationOptions.max;
		}
	}

	var numSpaces = (niceMax - niceMin) / spacing;
	// If very close to our rounded value, use it.
	if (helpers.almostEquals(numSpaces, Math.round(numSpaces), spacing / 1000)) {
		numSpaces = Math.round(numSpaces);
	} else {
		numSpaces = Math.ceil(numSpaces);
	}

	if (spacing < 1) {
		factor = Math.pow(10, spacing.toString().length - 2);
		niceMin = Math.round(niceMin * factor) / factor;
		niceMax = Math.round(niceMax * factor) / factor;
	}
	ticks.push(generationOptions.min !== undefined ? generationOptions.min : niceMin);
	for (var j = 1; j < numSpaces; ++j) {
		ticks.push(Math.round((niceMin + j * spacing) * factor) / factor);
	}
	ticks.push(generationOptions.max !== undefined ? generationOptions.max : niceMax);

	return ticks;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think precision is more understandable to end users. E.g. if you want to round to the nearest .01 it's pretty easy to see that this is two decimal places, but I'm not sure we would want to make the user calculate that they should pass in 100

Copy link
Contributor

Choose a reason for hiding this comment

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

@benmccann I am not sure I understand your comment. My proposal wasn't to change the precision-option given by the user, but to use the variable precision inside the function, instead of introducing an extra variable spacingPrecision.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or more precisely renaming the previous use of precision to factor and use this for both tick spacing as tick rounding, as they both serve the same purpose.

Copy link
Member

@simonbrunel simonbrunel Jan 24, 2018

Choose a reason for hiding this comment

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

I agree with @jcopperfield, it's more readable renaming internal variables precision -> factor and spacingPrecision -> precision since these have different units / meanings.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, will make this change

@saadismail
Copy link

In which version will this PR will be live in the stable version? I can see that this was merged into master back in Apr 2018 but https://www.chartjs.org/docs/latest/axes/cartesian/linear.html is still outdated and current version (2.8.0) does not have this.

@benmccann
Copy link
Contributor

@saadismail I'm not sure why you say https://www.chartjs.org/docs/latest/axes/cartesian/linear.html is outdated. I see "precision" on that page as was added in this PR

@saadismail
Copy link

@benmccann I am sorry, I did not read the whole conversation in the older thread and tried to look for integerSteps instead of precision. Thank you for the help and sorry for the inconvenience.

exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
If defined and `stepSize` is not specified, the step size will be rounded to this many decimal places.
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.

Only allow integer steps on axis
5 participants