Fix for issues #3917, #4380, #2966 - log scale with min: 0 by jcopperfield · Pull Request #4913 · chartjs/Chart.js

jcopperfield · 2017-11-01T15:35:11Z

Fixes #3917: Logarithmic Scale Tick Bug for values of 0
Fixes #4380: LogLog plots are impossible (0, 0)
Fixes #2966: chart does not render if data contains value of 0 when using logarithmic scale

v2.7

doesn't correctly render: horizontal with min:0 (regular and reversed)
in case the significand digit of minNotZero is 1 or 2;
however the value itself has more significand digits,
the first tick is displayed outside the chart area or on top of the 0-tick.

Test for all bar plot possibilities (vertical/horizontal, with/without min: 0, default/reversed)
https://codepen.io/anon/pen/zPYMWz

Examples

- issue chartjs#3917: Logarithmic Scale Tick Bug for values of 0 - issue chartjs#4380: LogLog plots are impossible (0, 0) - issue chartjs#2966: chart does not render if data contains value of 0 when using logarithmic scale

benmccann · 2017-11-03T03:48:18Z

-					pixel = me.left + (innerDimension / range * (helpers.log10(newVal) - helpers.log10(start)));
-				}
+				innerDimension = me.width;
+				pixel = (reverse)


Probably don't need to wrap reverse in parentheses. I also would probably put this on just one line given that it wouldn't be too long of a line

IMHO It improves readability.
I don't however want to start a discussion over code style, so if it is a must I will adjust the code as requested.

I agree with @benmccann, I would use parenthesis only when it's needed but also I'm not sure useless parenthesis are removed by the minifier.

About the "one line" style, it's not a "must" (else it would be enforced) but most of the code uses the a ? b : c form when a, b and c are short enough to make easier to read when on one line (which, I agree, is not necessary perceived the same by everyone).

benmccann · 2017-11-03T16:46:45Z

See also #4688

jcopperfield · 2017-11-03T18:05:31Z

I left a comment in PR #4688, since that code doesn't handle all use cases correctly.

simonbrunel

Looks good, a few minor comments. I don't know this part of the code, so didn't verify the logic.

simonbrunel · 2017-11-06T09:18:13Z

-					pixel = me.left + (innerDimension / range * (helpers.log10(newVal) - helpers.log10(start)));
-				}
+				innerDimension = me.width;
+				pixel = (reverse)


I agree with @benmccann, I would use parenthesis only when it's needed but also I'm not sure useless parenthesis are removed by the minifier.

About the "one line" style, it's not a "must" (else it would be enforced) but most of the code uses the a ? b : c form when a, b and c are short enough to make easier to read when on one line (which, I agree, is not necessary perceived the same by everyone).

simonbrunel · 2017-11-06T09:21:35Z

-			var innerDimension, pixel, range;
+			var realValue = +me.getRightValue(value);
+			var reverse = me.options.ticks.reverse;
+			var lg = helpers.log10;


I would rename it log10 to make it more obvious when reading line 227

simonbrunel · 2017-11-06T09:24:10Z

+			var baseValue, innerDimension, pixel, range, sign;

+			if (reverse) {
+				range = {


Can we avoid nesting start and end under range? (faster, lighter and easier to read)

start = me.end; end = me.start; sign = -1;

simonbrunel · 2017-11-06T09:26:35Z

+			var exp = Math.floor(lg(me.minNotZero));
+			var significand = Math.floor(me.minNotZero / Math.pow(10, exp));
+			var firstTickValue = significand * Math.pow(10, exp);
+			var range = (reverse)


Same here, can we avoid nesting start and end under range?

The minifier reduces the code to r=d?l.right:l.left, removing the parenthesis.

Being dyslexic, I have trouble reading lines like the following from the function determineDataLimits:
me.min = me.min === null ? minVal : Math.min(me.min, minVal);
For however short, every time it takes me about 10 seconds to understand what it does.
Expanding it over multiple lines and adding parenthesis removes this readability obstacle.

me.min = (me.min === null) ? minVal : Math.min(me.min, minVal);

Still it is just a personal preference.

It's really minor and I don't have a strong opinion on which form is better (I usually go for the one which is consistent with the rest of the code if that makes sense).

Anyway, that wasn't my point for this line, but to flatten the useless range object:

var start = reverse ? me.end : me.start; var end = reverse ? me.start : me.end;

simonbrunel · 2017-11-06T09:36:41Z

Seems you "unlocked" a few unsupported cases, should we add new unit tests for them or the existing ones will be enough (i.e. empty charts in your screenshots)?

…rPixel Add: Tests for more extensive data containing zero values or axes configuration containing 'min: 0'-option.

jcopperfield · 2017-11-06T15:43:57Z

The logarithmic tick generator creates different values for the first tick for data with 0 values and those without.

Example
For data with minimum value {x: 6.3, y: 6.3}, the first tick will be 1, however when for the same data the option min: 0 is set, the first tick will be 6.
The scale copies this behavior, however it might be better to be more consistent or to make it configurable.

simonbrunel

Looks good, I would simply fix the jsdoc block and prefix the private method with _, then I think it's good to be merged.

simonbrunel · 2017-11-09T21:48:55Z

+		 *
+		 * @Private
+		 * @param {Number} [value] - The minimum not zero value.
+		 * @return {Number} [firstTickValue] - The first tick value.


Should be @param {Number} value - The ... since value is not optional (http://usejsdoc.org/tags-param.html)
Should be @return {Number} The first ..., without name and separator (http://usejsdoc.org/tags-returns.html)
Can you also move @private at the end and only lowercase, and I would remove the extra empty line:

/** * Returns the value of the first tick. * @param {Number} value - The minimum not zero value. * @return {Number} The first tick value. * @private */ _getFirstTickValue: function(value) {

Similar to https://github.com/chartjs/Chart.js/blob/master/src/core/core.plugin.js#L92

Also we started to prefix private members by _ (for example), though it's not yet consistent.

simonbrunel · 2017-11-10T08:17:05Z

Thanks @jcopperfield

Fix for issues chartjs#3917, chartjs#4380, chartjs#2966

2b7421d

- issue chartjs#3917: Logarithmic Scale Tick Bug for values of 0 - issue chartjs#4380: LogLog plots are impossible (0, 0) - issue chartjs#2966: chart does not render if data contains value of 0 when using logarithmic scale

benmccann added the type: bug label Nov 2, 2017

benmccann reviewed Nov 3, 2017

View reviewed changes

etimberg approved these changes Nov 5, 2017

View reviewed changes

simonbrunel requested changes Nov 6, 2017

View reviewed changes

simonbrunel added this to the Version 2.8 milestone Nov 6, 2017

jcopperfield added 2 commits November 6, 2017 14:10

Add: Requested code style changes for getPixelForValue and getValueFo…

4de3227

…rPixel Add: Tests for more extensive data containing zero values or axes configuration containing 'min: 0'-option.

Fix: could not minify due to invalid function declaration

165ad97

Fix: renamed variables to improve readability

3519612

benmccann mentioned this pull request Nov 8, 2017

Logarithmic scale not working with 0 values #4934

Closed

simonbrunel requested changes Nov 9, 2017

View reviewed changes

Fix: prefix private function with _ and correct jsdocs

c5e24a6

simonbrunel approved these changes Nov 10, 2017

View reviewed changes

simonbrunel merged commit 939756c into chartjs:master Nov 10, 2017

jcopperfield deleted the PR-20171101-3917 branch November 10, 2017 08:41

benmccann mentioned this pull request Nov 12, 2017

fix for Logarithmic Scale Tick Bug for values of 0 #4688

Closed

yofreke pushed a commit to yofreke/Chart.js that referenced this pull request Dec 30, 2017

Fix log scale when value is 0 (chartjs#4913)

07e30c9

kurkle mentioned this pull request Jan 13, 2020

Fix log scale calculations #6903

Merged

snyk-bot mentioned this pull request Mar 9, 2020

[Snyk] Upgrade chart.js from 2.7.1 to 2.9.3 savinovvu/reg-exp#5

Closed

snyk-bot mentioned this pull request Apr 1, 2020

[Snyk] Upgrade chart.js from 2.7.2 to 2.9.3 baophucct/wallet#4

Open

exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021

Fix log scale when value is 0 (chartjs#4913)

85c4ce1

flesher1 mentioned this pull request Sep 8, 2021

Zero values skipped on logarithmic scale line chart #9629

Open

Conversation

jcopperfield commented Nov 1, 2017 • edited by simonbrunel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benmccann commented Nov 3, 2017

Uh oh!

jcopperfield commented Nov 3, 2017

Uh oh!

simonbrunel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonbrunel commented Nov 6, 2017

Uh oh!

jcopperfield commented Nov 6, 2017

Uh oh!

simonbrunel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonbrunel commented Nov 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jcopperfield commented Nov 1, 2017 •

edited by simonbrunel

Loading