From: Jukka Kurkela Date: Tue, 14 Jan 2020 00:36:50 +0000 (+0200) Subject: Fix log scale calculations (#6903) X-Git-Tag: v3.0.0-alpha~129 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=f1817979a0c9799acebb3b847a6535a48850c0db;p=thirdparty%2FChart.js.git Fix log scale calculations (#6903) * Fix log scale calculations * Fully remove _valueOffset * Invalidate 0 * Review update * Skip NaN Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com> --- diff --git a/docs/getting-started/v3-migration.md b/docs/getting-started/v3-migration.md index 986d03221..c5e39e11d 100644 --- a/docs/getting-started/v3-migration.md +++ b/docs/getting-started/v3-migration.md @@ -102,6 +102,7 @@ Animation system was completely rewritten in Chart.js v3. Each property can now * `ILayoutItem.minSize` * `IPlugin.afterScaleUpdate`. Use `afterLayout` instead * `Line.calculatePointY` +* `LogarithmicScale.minNotZero` * `Scale.getRightValue` * `Scale.handleDirectionalChanges` is now private * `Scale.longestLabelWidth` @@ -117,6 +118,7 @@ Animation system was completely rewritten in Chart.js v3. Each property can now * `Element._ctx` * `Element._model` * `Element._view` +* `LogarithmicScale._valueOffset` * `TimeScale._getPixelForOffset` * `TimeScale.getLabelWidth` * `Tooltip._lastActive` diff --git a/src/core/core.datasetController.js b/src/core/core.datasetController.js index db0bcd736..d89e1af0b 100644 --- a/src/core/core.datasetController.js +++ b/src/core/core.datasetController.js @@ -652,11 +652,10 @@ helpers.extend(DatasetController.prototype, { const ilen = _parsed.length; const otherScale = this._getOtherScale(scale); const stack = canStack && meta._stacked && {keys: getSortedDatasetIndices(this.chart, true), values: null}; + let min = Number.POSITIVE_INFINITY; let max = Number.NEGATIVE_INFINITY; let {min: otherMin, max: otherMax} = getUserBounds(otherScale); - let i, item, value, parsed, min, minPositive, otherValue; - - min = minPositive = Number.POSITIVE_INFINITY; + let i, item, value, parsed, otherValue; function _compute() { if (stack) { @@ -669,9 +668,6 @@ helpers.extend(DatasetController.prototype, { } min = Math.min(min, value); max = Math.max(max, value); - if (value > 0) { - minPositive = Math.min(minPositive, value); - } } function _skip() { @@ -700,7 +696,7 @@ helpers.extend(DatasetController.prototype, { break; } } - return {min, max, minPositive}; + return {min, max}; }, /** diff --git a/src/core/core.scale.js b/src/core/core.scale.js index 2f9bfce96..a68745c98 100644 --- a/src/core/core.scale.js +++ b/src/core/core.scale.js @@ -277,7 +277,6 @@ class Scale extends Element { _getMinMax(canStack) { const me = this; let {min, max, minDefined, maxDefined} = me._getUserBounds(); - let minPositive = Number.POSITIVE_INFINITY; let i, ilen, metas, minmax; if (minDefined && maxDefined) { @@ -293,10 +292,9 @@ class Scale extends Element { if (!maxDefined) { max = Math.max(max, minmax.max); } - minPositive = Math.min(minPositive, minmax.minPositive); } - return {min, max, minPositive}; + return {min, max}; } _invalidateCaches() {} diff --git a/src/scales/scale.logarithmic.js b/src/scales/scale.logarithmic.js index 28e9499a8..d44963a41 100644 --- a/src/scales/scale.logarithmic.js +++ b/src/scales/scale.logarithmic.js @@ -1,20 +1,20 @@ 'use strict'; -import defaults from '../core/core.defaults'; -import helpers from '../helpers/index'; -import {_setMinAndMaxByKey} from '../helpers/helpers.math'; +import {isFinite} from '../helpers/helpers.core'; +import {_setMinAndMaxByKey, log10} from '../helpers/helpers.math'; import Scale from '../core/core.scale'; import LinearScaleBase from './scale.linearbase'; import Ticks from '../core/core.ticks'; -const valueOrDefault = helpers.valueOrDefault; -const log10 = helpers.math.log10; - function isMajor(tickVal) { const remain = tickVal / (Math.pow(10, Math.floor(log10(tickVal)))); return remain === 1; } +function finiteOrDefault(value, def) { + return isFinite(value) ? value : def; +} + /** * Generate a set of logarithmic ticks * @param generationOptions the options used to generate the ticks @@ -25,16 +25,9 @@ function generateTicks(generationOptions, dataRange) { const endExp = Math.floor(log10(dataRange.max)); const endSignificand = Math.ceil(dataRange.max / Math.pow(10, endExp)); const ticks = []; - let tickVal = valueOrDefault(generationOptions.min, Math.pow(10, Math.floor(log10(dataRange.min)))); - let exp, significand; - - if (tickVal === 0) { - exp = 0; - significand = 0; - } else { - exp = Math.floor(log10(tickVal)); - significand = Math.floor(tickVal / Math.pow(10, exp)); - } + let tickVal = finiteOrDefault(generationOptions.min, Math.pow(10, Math.floor(log10(dataRange.min)))); + let exp = Math.floor(log10(tickVal)); + let significand = Math.floor(tickVal / Math.pow(10, exp)); let precision = exp < 0 ? Math.pow(10, Math.abs(exp)) : 1; do { @@ -50,7 +43,7 @@ function generateTicks(generationOptions, dataRange) { tickVal = Math.round(significand * Math.pow(10, exp) * precision) / precision; } while (exp < endExp || (exp === endExp && significand < endSignificand)); - const lastTick = valueOrDefault(generationOptions.max, tickVal); + const lastTick = finiteOrDefault(generationOptions.max, tickVal); ticks.push({value: lastTick, major: isMajor(tickVal)}); return ticks; @@ -69,7 +62,10 @@ const defaultConfig = { class LogarithmicScale extends Scale { _parse(raw, index) { // eslint-disable-line no-unused-vars const value = LinearScaleBase.prototype._parse.apply(this, arguments); - return helpers.isFinite(value) && value >= 0 ? value : undefined; + if (value === 0) { + return undefined; + } + return isFinite(value) && value > 0 ? value : NaN; } determineDataLimits() { @@ -77,11 +73,9 @@ class LogarithmicScale extends Scale { const minmax = me._getMinMax(true); const min = minmax.min; const max = minmax.max; - const minPositive = minmax.minPositive; - me.min = helpers.isFinite(min) ? Math.max(0, min) : null; - me.max = helpers.isFinite(max) ? Math.max(0, max) : null; - me.minNotZero = helpers.isFinite(minPositive) ? minPositive : null; + me.min = isFinite(min) ? Math.max(0, min) : null; + me.max = isFinite(max) ? Math.max(0, max) : null; me.handleTickRangeOptions(); } @@ -94,30 +88,19 @@ class LogarithmicScale extends Scale { let max = me.max; if (min === max) { - if (min !== 0 && min !== null) { - min = Math.pow(10, Math.floor(log10(min)) - 1); - max = Math.pow(10, Math.floor(log10(max)) + 1); - } else { + if (min <= 0) { // includes null min = DEFAULT_MIN; max = DEFAULT_MAX; + } else { + min = Math.pow(10, Math.floor(log10(min)) - 1); + max = Math.pow(10, Math.floor(log10(max)) + 1); } } - if (min === null) { + if (min <= 0) { min = Math.pow(10, Math.floor(log10(max)) - 1); } - if (max === null) { - max = min !== 0 - ? Math.pow(10, Math.floor(log10(min)) + 1) - : DEFAULT_MAX; - } - if (me.minNotZero === null) { - if (min > 0) { - me.minNotZero = min; - } else if (max < 1) { - me.minNotZero = Math.pow(10, Math.floor(log10(max))); - } else { - me.minNotZero = DEFAULT_MIN; - } + if (max <= 0) { + max = Math.pow(10, Math.floor(log10(min)) + 1); } me.min = min; me.max = max; @@ -152,6 +135,10 @@ class LogarithmicScale extends Scale { return ticks; } + getLabelForValue(value) { + return value === undefined ? 0 : value; + } + getPixelForTick(index) { const ticks = this.ticks; if (index < 0 || index > ticks.length - 1) { @@ -160,52 +147,30 @@ class LogarithmicScale extends Scale { return this.getPixelForValue(ticks[index].value); } - /** - * Returns the value of the first tick. - * @param {number} value - The minimum not zero value. - * @return {number} The first tick value. - * @private - */ - _getFirstTickValue(value) { - const exp = Math.floor(log10(value)); - const significand = Math.floor(value / Math.pow(10, exp)); - - return significand * Math.pow(10, exp); - } - _configure() { const me = this; let start = me.min; - let offset = 0; Scale.prototype._configure.call(me); - if (start === 0) { - start = me._getFirstTickValue(me.minNotZero); - offset = valueOrDefault(me.options.ticks.fontSize, defaults.fontSize) / me._length; - } - me._startValue = log10(start); - me._valueOffset = offset; - me._valueRange = (log10(me.max) - log10(start)) / (1 - offset); + me._valueRange = log10(me.max) - log10(start); } getPixelForValue(value) { const me = this; - let decimal = 0; - - if (value > me.min && value > 0) { - decimal = (log10(value) - me._startValue) / me._valueRange + me._valueOffset; + if (value === undefined || value === 0) { + value = me.min; } - return me.getPixelForDecimal(decimal); + return me.getPixelForDecimal(value === me.min + ? 0 + : (log10(value) - me._startValue) / me._valueRange); } getValueForPixel(pixel) { const me = this; const decimal = me.getDecimalForPixel(pixel); - return decimal === 0 && me.min === 0 - ? 0 - : Math.pow(10, me._startValue + (decimal - me._valueOffset) * me._valueRange); + return Math.pow(10, me._startValue + decimal * me._valueRange); } } diff --git a/test/fixtures/controller.bar/stacking/logarithmic-strings.png b/test/fixtures/controller.bar/stacking/logarithmic-strings.png index 2ce8d067c..52dae9b93 100644 Binary files a/test/fixtures/controller.bar/stacking/logarithmic-strings.png and b/test/fixtures/controller.bar/stacking/logarithmic-strings.png differ diff --git a/test/fixtures/controller.bar/stacking/logarithmic.png b/test/fixtures/controller.bar/stacking/logarithmic.png index 2ce8d067c..52dae9b93 100644 Binary files a/test/fixtures/controller.bar/stacking/logarithmic.png and b/test/fixtures/controller.bar/stacking/logarithmic.png differ diff --git a/test/specs/scale.logarithmic.tests.js b/test/specs/scale.logarithmic.tests.js index 6fa8a1280..567bdcee4 100644 --- a/test/specs/scale.logarithmic.tests.js +++ b/test/specs/scale.logarithmic.tests.js @@ -105,11 +105,11 @@ describe('Logarithmic Scale tests', function() { expect(chart.scales.y1.max).toBe(5000); expect(chart.scales.y2).not.toEqual(undefined); // must construct - expect(chart.scales.y2.min).toBe(0); + expect(chart.scales.y2.min).toBe(10); expect(chart.scales.y2.max).toBe(4000); expect(chart.scales.y3).not.toEqual(undefined); // must construct - expect(chart.scales.y3.min).toBe(0); + expect(chart.scales.y3.min).toBeCloseTo(0.0001, 4); expect(chart.scales.y3.max).toBe(900); }); @@ -165,11 +165,11 @@ describe('Logarithmic Scale tests', function() { expect(chart.scales.y1.max).toBe(5000); expect(chart.scales.y2).not.toEqual(undefined); // must construct - expect(chart.scales.y2.min).toBe(0); + expect(chart.scales.y2.min).toBe(10); expect(chart.scales.y2.max).toBe(4000); expect(chart.scales.y3).not.toEqual(undefined); // must construct - expect(chart.scales.y3.min).toBe(0); + expect(chart.scales.y3.min).toBeCloseTo(0.0001, 4); expect(chart.scales.y3.max).toBe(900); }); @@ -219,7 +219,7 @@ describe('Logarithmic Scale tests', function() { expect(chart.scales.y1.max).toBe(5000); expect(chart.scales.y2).not.toEqual(undefined); // must construct - expect(chart.scales.y2.min).toBe(0); + expect(chart.scales.y2.min).toBe(10); expect(chart.scales.y2.max).toBe(8000); }); @@ -267,7 +267,7 @@ describe('Logarithmic Scale tests', function() { expect(chart.scales.y.max).toBe(6000); expect(chart.scales.y1).not.toEqual(undefined); // must construct - expect(chart.scales.y1.min).toBe(0); + expect(chart.scales.y1.min).toBe(1); expect(chart.scales.y1.max).toBe(10000); }); @@ -330,10 +330,10 @@ describe('Logarithmic Scale tests', function() { } }); - expect(chart.scales.x.min).toBe(0); - expect(chart.scales.x.max).toBe(300); + expect(chart.scales.x.min).toBe(1); + expect(chart.scales.x.max).toBe(30); - expect(chart.scales.y.min).toBe(0); + expect(chart.scales.y.min).toBe(0.01); expect(chart.scales.y.max).toBe(1000); }); @@ -506,7 +506,7 @@ describe('Logarithmic Scale tests', function() { }); var y = chart.scales.y; - expect(y.min).toBe(0); + expect(y.min).toBe(1); expect(y.max).toBe(2); }); @@ -536,7 +536,7 @@ describe('Logarithmic Scale tests', function() { }); var y = chart.scales.y; - expect(y.min).toBe(0); + expect(y.min).toBe(1); expect(y.max).toBe(2); }); @@ -595,8 +595,8 @@ describe('Logarithmic Scale tests', function() { var scale = chart.scales.y; // Counts down because the lines are drawn top to bottom - expect(getLabels(scale)).toEqual([30, 20, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0]); - expect(scale.start).toEqual(0); + expect(getLabels(scale)).toEqual([30, 20, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0.9, 0.8, 0.7, 0.6, 0.5, 0.4, 0.3, 0.2, 0.1]); + expect(scale.start).toEqual(0.1); expect(scale.end).toEqual(30); }); @@ -656,9 +656,9 @@ describe('Logarithmic Scale tests', function() { }); var scale = chart.scales.y; - expect(getLabels(scale)).toEqual([0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 20, 30]); + expect(getLabels(scale)).toEqual([1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 20, 30]); expect(scale.start).toEqual(30); - expect(scale.end).toEqual(0); + expect(scale.end).toEqual(1); }); it('should build labels using the default template', function() { @@ -679,7 +679,7 @@ describe('Logarithmic Scale tests', function() { } }); - expect(getLabels(chart.scales.y)).toEqual(['8e+1', '7e+1', '6e+1', '5e+1', '4e+1', '3e+1', '2e+1', '1e+1', '9e+0', '8e+0', '7e+0', '6e+0', '5e+0', '4e+0', '3e+0', '2e+0', '1e+0', '0']); + expect(getLabels(chart.scales.y)).toEqual(['8e+1', '7e+1', '6e+1', '5e+1', '4e+1', '3e+1', '2e+1', '1e+1', '9e+0', '8e+0', '7e+0', '6e+0', '5e+0', '4e+0', '3e+0', '2e+0', '1e+0']); }); it('should build labels using the user supplied callback', function() { @@ -779,7 +779,7 @@ describe('Logarithmic Scale tests', function() { min: 0 } }, - firstTick: 0, + firstTick: 1, describe: 'all stacks are defined and min: 0' }, { @@ -790,7 +790,7 @@ describe('Logarithmic Scale tests', function() { min: 0 } }, - firstTick: 0, + firstTick: 1, describe: 'not stacks are defined and min: 0' }, { @@ -811,7 +811,7 @@ describe('Logarithmic Scale tests', function() { min: 0 } }, - firstTick: 0, + firstTick: 1, describe: 'all stacks are defined and min: 0' }, { @@ -822,7 +822,7 @@ describe('Logarithmic Scale tests', function() { min: 0 } }, - firstTick: 0, + firstTick: 1, describe: 'not all stacks are defined and min: 0' }, ]; @@ -1011,9 +1011,9 @@ describe('Logarithmic Scale tests', function() { var start = chart.chartArea[axis.start]; var end = chart.chartArea[axis.end]; - expect(scale.getPixelForValue(firstTick, 0, 0)).toBeCloseToPixel(start); - expect(scale.getPixelForValue(lastTick, 0, 0)).toBeCloseToPixel(end); - expect(scale.getPixelForValue(0, 0, 0)).toBeCloseToPixel(start); // 0 is invalid, put it at the start. + expect(scale.getPixelForValue(firstTick)).toBeCloseToPixel(start); + expect(scale.getPixelForValue(lastTick)).toBeCloseToPixel(end); + expect(scale.getPixelForValue(0)).toBeCloseToPixel(start); // 0 is invalid, put it at the start. expect(scale.getValueForPixel(start)).toBeCloseTo(firstTick, 4); expect(scale.getValueForPixel(end)).toBeCloseTo(lastTick, 4); @@ -1025,8 +1025,8 @@ describe('Logarithmic Scale tests', function() { start = chart.chartArea[axis.end]; end = chart.chartArea[axis.start]; - expect(scale.getPixelForValue(firstTick, 0, 0)).toBeCloseToPixel(start); - expect(scale.getPixelForValue(lastTick, 0, 0)).toBeCloseToPixel(end); + expect(scale.getPixelForValue(firstTick)).toBeCloseToPixel(start); + expect(scale.getPixelForValue(lastTick)).toBeCloseToPixel(end); expect(scale.getValueForPixel(start)).toBeCloseTo(firstTick, 4); expect(scale.getValueForPixel(end)).toBeCloseTo(lastTick, 4); @@ -1041,47 +1041,40 @@ describe('Logarithmic Scale tests', function() { { dataset: [], scale: {min: 0}, - firstTick: 1, // value of the first tick lastTick: 10, // value of the last tick describe: 'empty dataset, min: 0, without max' }, { dataset: [], scale: {min: 0, max: 80}, - firstTick: 1, lastTick: 80, describe: 'empty dataset, min: 0, max: 80' }, { dataset: [], scale: {min: 0, max: 0.8}, - firstTick: 0.1, lastTick: 0.8, describe: 'empty dataset, min: 0, max: 0.8' }, { dataset: [{x: 0, y: 0}, {x: 10, y: 10}, {x: 1.2, y: 1.2}, {x: 25, y: 25}, {x: 78, y: 78}], - firstTick: 1, lastTick: 80, - describe: 'dataset min point {x: 0, y: 0}, max point {x:78, y:78}, minNotZero {x: 1.2, y: 1.2}' + describe: 'dataset min point {x: 0, y: 0}, max point {x:78, y:78}' }, { dataset: [{x: 0, y: 0}, {x: 10, y: 10}, {x: 6.3, y: 6.3}, {x: 25, y: 25}, {x: 78, y: 78}], - firstTick: 6, lastTick: 80, - describe: 'dataset min point {x: 0, y: 0}, max point {x:78, y:78}, minNotZero {x: 6.3, y: 6.3}' + describe: 'dataset min point {x: 0, y: 0}, max point {x:78, y:78}' }, { dataset: [{x: 10, y: 10}, {x: 1.2, y: 1.2}, {x: 25, y: 25}, {x: 78, y: 78}], scale: {min: 0}, - firstTick: 1, lastTick: 80, describe: 'dataset min point {x: 1.2, y: 1.2}, max point {x:78, y:78}, min: 0' }, { dataset: [{x: 10, y: 10}, {x: 6.3, y: 6.3}, {x: 25, y: 25}, {x: 78, y: 78}], scale: {min: 0}, - firstTick: 6, lastTick: 80, describe: 'dataset min point {x: 6.3, y: 6.3}, max point {x:78, y:78}, min: 0' }, @@ -1100,7 +1093,7 @@ describe('Logarithmic Scale tests', function() { } ]; axes.forEach(function(axis) { - var expectation = 'min = 0, max = ' + setup.lastTick + ', first tick = ' + setup.firstTick; + var expectation = 'min = 0, max = ' + setup.lastTick; describe(setup.describe + ' and axis is "' + axis.id + '"; expect: ' + expectation + ';', function() { beforeEach(function() { var xConfig = { @@ -1135,20 +1128,15 @@ describe('Logarithmic Scale tests', function() { var chart = this.chart; var axisID = axis.id; var scale = chart.scales[axisID]; - var firstTick = setup.firstTick; var lastTick = setup.lastTick; - var fontSize = chart.options.fontSize; var start = chart.chartArea[axis.start]; var end = chart.chartArea[axis.end]; - var sign = scale.isHorizontal() ? 1 : -1; - expect(scale.getPixelForValue(0, 0, 0)).toBeCloseToPixel(start); - expect(scale.getPixelForValue(lastTick, 0, 0)).toBeCloseToPixel(end); - expect(scale.getPixelForValue(firstTick, 0, 0)).toBeCloseToPixel(start + sign * fontSize); + expect(scale.getPixelForValue(0)).toBeCloseToPixel(start); + expect(scale.getPixelForValue(lastTick)).toBeCloseToPixel(end); - expect(scale.getValueForPixel(start)).toBeCloseTo(0, 4); + expect(scale.getValueForPixel(start)).toBeCloseTo(scale.min, 4); expect(scale.getValueForPixel(end)).toBeCloseTo(lastTick, 4); - expect(scale.getValueForPixel(start + sign * fontSize)).toBeCloseTo(firstTick, 4); chart.scales[axisID].options.reverse = true; // Reverse mode chart.update(); @@ -1157,13 +1145,11 @@ describe('Logarithmic Scale tests', function() { start = chart.chartArea[axis.end]; end = chart.chartArea[axis.start]; - expect(scale.getPixelForValue(0, 0, 0)).toBeCloseToPixel(start); - expect(scale.getPixelForValue(lastTick, 0, 0)).toBeCloseToPixel(end); - expect(scale.getPixelForValue(firstTick, 0, 0)).toBeCloseToPixel(start - sign * fontSize, 4); + expect(scale.getPixelForValue(0)).toBeCloseToPixel(start); + expect(scale.getPixelForValue(lastTick)).toBeCloseToPixel(end); - expect(scale.getValueForPixel(start)).toBeCloseTo(0, 4); + expect(scale.getValueForPixel(start)).toBeCloseTo(scale.min, 4); expect(scale.getValueForPixel(end)).toBeCloseTo(lastTick, 4); - expect(scale.getValueForPixel(start - sign * fontSize)).toBeCloseTo(firstTick, 4); }); }); });