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

Fix issue #4928: linear tick generator doesn't round values to needed precision. #4943

Merged
merged 5 commits into from
Dec 2, 2017

Conversation

jcopperfield
Copy link
Contributor

@jcopperfield jcopperfield commented Nov 13, 2017

Fixes #4928 - linear tick generator doesn't round values to needed precision.

Test case: https://codepen.io/anon/pen/LOLxvL

v2.7
issue_tick_rounding_error_4928
Proposed fix
fix-issue_tick_rounding_error_4928

 - linear tick generator doesn't round values to needed precision.
// Put the values into the ticks array
var precision = 1;
if (spacing < 1) {
precision = Math.pow(10, spacing.toPrecision().length - 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be equivalent to replace .toPrecision() with .toString()? I think that would make it clearer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case I think they both return equal results. Since the code is trying to find the number of significant digits after the decimal point.

@benmccann
Copy link
Contributor

You're on a roll! This is great!

@jcopperfield
Copy link
Contributor Author

jcopperfield commented Nov 14, 2017

The logarithmic tick generator contains the same problem.
v2.7
issue_logarithmic_tick_rounding_error
PR Fix: logarithmic tick generator doesn't round values to needed precision ad984be
fix_issue_logarithmic_tick_rounding_error png

@simonbrunel simonbrunel added this to the Version 2.8 milestone Nov 14, 2017
@jcopperfield
Copy link
Contributor Author

Rounding negative values didn't create the expected result
v.2.7
issue_rounding_negative_tick_values
PR Fix: rounding tick values didn't work for negative values. 37bfadb
fix_issue_rounding_negative_tick_values

@simonbrunel
Copy link
Member

Could be useful to unit test these expected values (positive and negative)?

@jcopperfield
Copy link
Contributor Author

jcopperfield commented Nov 14, 2017

Where is such a test best placed, and should the test be run directly on the generators.linear function or on a chart?

BTW a lot of test code use the function toBeCloseTo like expect(number).toBeCloseTo(10, 1e-4), however jasmine documentation describes the second parameter as precisionopt:

toBeCloseTo(expected, precisionopt)
expect the actual value to be within a specified precision of the expected value.
Parameters:

Name Type Attributes Default Description
expected Object     The expected value to compare against.
precision Number 2 The number of decimal points to check.

example
expect(number).toBeCloseTo(42.2, 3);

@simonbrunel
Copy link
Member

I don't think tests can run directly on generators because they are not accessible from Chart.*, so it would require to generate a chart and test generated values from ticks (as all other tests). I would still keep these tests in a new core.ticks.tests.js to make easier source/test mapping (as I would do for the layout PR).

Not sure why toBeCloseTo(10, 1e-4) (@etimberg), might be a good idea to update those checks with proper values.

@etimberg
Copy link
Member

Agree about testing on a real chart.

For the toBeCloseTo I must have misread the documentation when it was added. Lets fix that

@etimberg etimberg merged commit 6f34b22 into chartjs:master Dec 2, 2017
@jcopperfield jcopperfield deleted the PR-20171113-4928 branch December 3, 2017 07:37
yofreke pushed a commit to yofreke/Chart.js that referenced this pull request Dec 30, 2017
… needed precision. (chartjs#4943)

* Fix issue 4928
 - linear tick generator doesn't round values to needed precision.

* Improve: replace toPrecision() in toString() to improve readability.

* Fix: logarithmic tick generator doesn't round values to needed precision.

* Fix: rounding tick values didn't work for negative values.

* Add: Core ticks tests
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
… needed precision. (chartjs#4943)

* Fix issue 4928
 - linear tick generator doesn't round values to needed precision.

* Improve: replace toPrecision() in toString() to improve readability.

* Fix: logarithmic tick generator doesn't round values to needed precision.

* Fix: rounding tick values didn't work for negative values.

* Add: Core ticks tests
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.

Line chart render yAxes error with number "0.6"
4 participants