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.

Sign up for GitHub

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

Jump to bottom

Fix log scale calculations #6903

Merged
merged 5 commits into from
Jan 14, 2020
Merged

Fix log scale calculations #6903

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2dab63a
Fix log scale calculations
benmccann Jan 4, 2020
c21002e
Fully remove _valueOffset
benmccann Jan 4, 2020
74e90d4
Invalidate 0
kurkle Jan 4, 2020
9e6c90b
Review update
kurkle Jan 5, 2020
80dab18
Skip NaN
kurkle Jan 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/getting-started/v3-migration.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,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`
Expand All @@ -114,6 +115,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`

Expand Down
10 changes: 3 additions & 7 deletions src/core/core.datasetController.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -635,11 +635,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) {
Expand All @@ -652,9 +651,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() {
Expand Down Expand Up @@ -683,7 +679,7 @@ helpers.extend(DatasetController.prototype, {
break;
}
}
return {min, max, minPositive};
return {min, max};
},

/**
Expand Down
4 changes: 1 addition & 3 deletions src/core/core.scale.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,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) {
Expand All @@ -309,10 +308,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() {}
Expand Down
103 changes: 34 additions & 69 deletions src/scales/scale.logarithmic.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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 {
Expand All @@ -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;
Expand All @@ -69,19 +62,20 @@ 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) {
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 I prefer calculating minPositive in _getMinMax rather than handling 0 specially here. (Actually probably what would be nicest is overriding _getMinMax in the log scale, but the _getMinMax logic has gotten so complicated that's a bit harder).

E.g. what if someone has a toggle between linear and log scales and wants to set parsing: false? I think the more we can keep scales out of the parsing business the better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Data with only positive values (and 0) works fine without parsing, if its in the correct format. One needs to set min value to scale manually then, but that would be expected when parsing=false, wouldn't it?
I have a follow-up to make beginAtZero work nicely on log scale, but I'm getting bored with this.

Copy link
Contributor

Choose a reason for hiding this comment

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

In my mind parsing is just that. It doesn't include validation or anything else. But I don't necessarily want to spend more time on this, so if you and @etimberg are both okay with this then so am I

return undefined;
}
return isFinite(value) && value > 0 ? value : NaN;
}

determineDataLimits() {
const me = this;
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();
}
Expand All @@ -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
kurkle marked this conversation as resolved.
Show resolved Hide resolved
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;
Expand Down Expand Up @@ -152,6 +135,10 @@ class LogarithmicScale extends Scale {
return ticks;
}

getLabelForValue(value) {
benmccann marked this conversation as resolved.
Show resolved Hide resolved
return value === undefined ? 0 : value;
}

getPixelForTick(index) {
const ticks = this.ticks;
if (index < 0 || index > ticks.length - 1) {
Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

can we return null here or something? this seems a little funny and I'm not quite sure why we're doing it

Copy link
Member Author

Choose a reason for hiding this comment

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

Returning min value of scale for 0 (and parsed 0 that will be undefined), so those values are drawn.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why use the scale min instead of undefined though? It seems strange to me to change it to a somewhat arbitrary different value rather than making it undefined

Copy link
Member Author

Choose a reason for hiding this comment

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

Lets make one thing clear, do you think we should continue to draw zero on a log chart or not?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say we should not

Copy link
Member Author

Choose a reason for hiding this comment

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

There are few fixed issues about that. #4913 fixed some for v2.7.1

If its not clear already, this is not something I want to spend my time with.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. I'm not sure how this will work in all cases, but if we need to change it later I suppose we can

}
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);
}
}

Expand Down
Binary file modified test/fixtures/controller.bar/stacking/logarithmic-strings.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/fixtures/controller.bar/stacking/logarithmic.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading