Improve axis tick increment #4070

archmoj · 2019-07-22T17:22:14Z

Fixes #3814.
This PR removes some precision errors happening during tick increments on axes.
Two patches are added here:
First and most importantly we use a dynamic pattern similar to (10 * 0.1 + 10 * 0.2) / 10 to get exactly 0.3 as the result not 0.30000000000000004.
Then we check if the length of new x (i.e. x0 + dtick) as an string is greater than the total lengths of previous value and dtick we round the result to 12 digits. This helps cover few edge cases e.g. where 3 * 0.3 sums up to 0.8999999999999999 not 0.9.
To view before vs after changes please view 4855033?short_path=4bd0c49#diff-4bd0c497a76299dd674f7b3092fb6aad.

Also to mention it appears that there is currently a bug in d3 formatting where 0.55 in p does not return a rounded 55%.
.

@plotly/plotly_js

antoinerg · 2019-07-22T18:15:31Z

Also to mention it appears that there is currently a bug in d3 formatting where 0.55 in p does not return a rounded 55%.

Just to be clear, this bug exists in our outdated version of d3-format. On newer versions, this bug is gone: https://runkit.com/embed/ooxgp8bijh5w

antoinerg · 2019-07-22T18:23:13Z

Thanks for this PR @archmoj ! It already improves things quite a bit.

Do you think it would be a lot of work to switch to the newer d3-format (which is its own little library) instead of using the built-in one from d3v3? grep -R d3.format src/ does not return that many occurrences.

archmoj · 2019-07-22T18:36:41Z

Thanks for this PR @archmoj ! It already improves things quite a bit.

Do you think it would be a lot of work to switch to the newer d3-format (which is its own little library) instead of using the built-in one from d3v3? grep -R d3.format src/ does not return that many occurrences.

Also to mention it appears that there is currently a bug in d3 formatting where 0.55 in p does not return a rounded 55%.

Just to be clear, this bug exists in our outdated version of d3-format. On newer versions, this bug is gone: https://runkit.com/embed/ooxgp8bijh5w

Thanks for the note. But that one still returns extra zeros!

archmoj · 2019-07-22T18:38:58Z

Thanks for this PR @archmoj ! It already improves things quite a bit.

Do you think it would be a lot of work to switch to the newer d3-format (which is its own little library) instead of using the built-in one from d3v3? grep -R d3.format src/ does not return that many occurrences.

Good question. However; I think we should try that on a separate branch/PR.
@antoinerg are you going to open a new issue for it?

antoinerg · 2019-07-22T19:11:43Z

Thanks for the note. But that one still returns extra zeros!

You're right! Sorry for the misinformation.

Good question. However; I think we should try that on a separate branch/PR.
@antoinerg are you going to open a new issue for it?

Sounds good. Well, if upgrading d3-format doesn't fix a bug, I think I can wait until we upgrade d3.

antoinerg · 2019-07-22T19:35:18Z

Ok, thank you @archmoj! I think I'll let @etpinard or @alexcjohnson review this one.

antoinerg · 2019-07-22T19:52:23Z

src/plots/cartesian/axes.js

+        // Note 1:
+        // 0.3 != 0.1 + 0.2 but 0.3 == ((10 * 0.1) + (10 * 0.2)) / 10
+        // Attempt to use integer steps to increment
+        var magic = 1 / dtick;


Do you have a reference explaining that "magic"?

Unfortunately no. But the trick appear to be working.

Hmm, do we really need this block for tickformat values other than 'p'?

Yes it is also required e.g. with s.
Please find the new mock added in 740574b which currently renders as:

Yes it is also required e.g. with s.

Ok so could then have

if(isNumeric(dtick) && (tickformat.indexOf('s') !== -1 || tickformat.indexOf('p') !== -1)) { // your new logic }

that would make this patch a lot less stressful to merge.

@antoinerg @etpinard the logic is now moved to Lib in 916bf80 and I improved test coverage in 86b135d.

…numbers

antoinerg · 2019-07-23T16:53:48Z

src/plots/cartesian/axes.js

+        var lenX0 = ('' + x).length;
+        var lenX1 = ('' + newX).length;
+
+        if(lenX1 > lenX0 + lenDt) { // this is likey a rounding error!


@archmoj Just a thought here: if you know the number of decimals in dtick, you also know that the number of decimals in the final number should never exceed it. So if newX.split('.')[1].length > dtickDecimalLength then you can round with a precision of dtickDecimalLength. I have the feeling it could fix the 55% but I may be wrong.

@antoinerg thanks for sharing this.
That one not only depends on the dtick but also on the previous tick value. For example if we start from 1000.0001 and dtick=1, the next tick should be at 1001.001.

Also I should clarify that the case of 55% is a d3 bug which is separated from what is going to be fixed in this PR i.e. providing better raw tick values.

That one not only depends on the dtick but also on the previous tick value. For example if we start from 1000.0001 and dtick=1, the next tick should be at 1001.001.

Wouldn't taking the maximal number of decimals in either dtick or the start number work?

force magic number to be positive to avoid getting -0

archmoj · 2019-07-23T18:24:12Z

src/lib/increment_numeric.js

+
+module.exports = function incrementNumeric(x, delta) {
+    if(!delta) return x;
+


One may simply return x + delta here to find out various failing tests.

etpinard · 2019-07-23T21:52:08Z

src/lib/increment_numeric.js

+        newX = +parseFloat(newX).toPrecision(12);
+    }
+
+    return newX;


Thanks for adding those tests @archmoj - I understand (a little bit) more what you're attempting.

That said, I feel like we should be extending numFormat:

plotly.js/src/plots/cartesian/axes.js

Lines 1311 to 1408 in 96fe262

function numFormat(v, ax, fmtoverride, hover) {

var isNeg = v < 0;

// max number of digits past decimal point to show

var tickRound = ax._tickround;

var exponentFormat = fmtoverride || ax.exponentformat || 'B';

var exponent = ax._tickexponent;

var tickformat = axes.getTickFormat(ax);

var separatethousands = ax.separatethousands;

// special case for hover: set exponent just for this value, and

// add a couple more digits of precision over tick labels

if(hover) {

// make a dummy axis obj to get the auto rounding and exponent

var ah = {

exponentformat: exponentFormat,

dtick: ax.showexponent === 'none' ? ax.dtick :

(isNumeric(v) ? Math.abs(v) || 1 : 1),

// if not showing any exponents, don't change the exponent

// from what we calculate

range: ax.showexponent === 'none' ? ax.range.map(ax.r2d) : [0, v || 1]

};

autoTickRound(ah);

tickRound = (Number(ah._tickround) || 0) + 4;

exponent = ah._tickexponent;

if(ax.hoverformat) tickformat = ax.hoverformat;

}

if(tickformat) return ax._numFormat(tickformat)(v).replace(/-/g, MINUS_SIGN);

// 'epsilon' - rounding increment

var e = Math.pow(10, -tickRound) / 2;

// exponentFormat codes:

// 'e' (1.2e+6, default)

// 'E' (1.2E+6)

// 'SI' (1.2M)

// 'B' (same as SI except 10^9=B not G)

// 'none' (1200000)

// 'power' (1.2x10^6)

// 'hide' (1.2, use 3rd argument=='hide' to eg

// only show exponent on last tick)

if(exponentFormat === 'none') exponent = 0;

// take the sign out, put it back manually at the end

// - makes cases easier

v = Math.abs(v);

if(v < e) {

// 0 is just 0, but may get exponent if it's the last tick

v = '0';

isNeg = false;

} else {

v += e;

// take out a common exponent, if any

if(exponent) {

v *= Math.pow(10, -exponent);

tickRound += exponent;

}

// round the mantissa

if(tickRound === 0) v = String(Math.floor(v));

else if(tickRound < 0) {

v = String(Math.round(v));

v = v.substr(0, v.length + tickRound);

for(var i = tickRound; i < 0; i++) v += '0';

} else {

v = String(v);

var dp = v.indexOf('.') + 1;

if(dp) v = v.substr(0, dp + tickRound).replace(/\.?0+$/, '');

}

// insert appropriate decimal point and thousands separator

v = Lib.numSeparate(v, ax._separators, separatethousands);

}

// add exponent

if(exponent && exponentFormat !== 'hide') {

if(isSIFormat(exponentFormat) && beyondSI(exponent)) exponentFormat = 'power';

var signedExponent;

if(exponent < 0) signedExponent = MINUS_SIGN + -exponent;

else if(exponentFormat !== 'power') signedExponent = '+' + exponent;

else signedExponent = String(exponent);

if(exponentFormat === 'e' || exponentFormat === 'E') {

v += exponentFormat + signedExponent;

} else if(exponentFormat === 'power') {

v += '×10<sup>' + signedExponent + '</sup>';

} else if(exponentFormat === 'B' && exponent === 9) {

v += 'B';

} else if(isSIFormat(exponentFormat)) {

v += SIPREFIXES[exponent / 3 + 5];

}

}

// put sign back in and return

// replace standard minus character (which is technically a hyphen)

// with a true minus sign

if(isNeg) return MINUS_SIGN + v;

return v;

}

where instead of exiting early

plotly.js/src/plots/cartesian/axes.js

Line 1338 in 96fe262

if(tickformat) return ax._numFormat(tickformat)(v).replace(/-/g, MINUS_SIGN);

for when tickformat is set, we could construct the formatted number ourselves for tickformat values that include s and p. To me, that sounds a lot more robust than these floating-point tricks that gets us a rounded-off tick step. What do you think?

I think it would be interesting to fix the issue at the root cause & thanks to JavaScript.
These rounding issues happen in other languages too.
Following images illustrate examples in Fortran & C++.

it would be interesting to fix the issue at the root cause

What is the root cause in your mind here?

alexcjohnson · 2019-07-30T04:03:10Z

@archmoj I did a little testing, and I think you may be on to something useful. AFAICT, as long as we add/subtract integers, and then divide by a power of 10 (again, explicitly rounded to be an integer), we're protected from these floating point errors over a quite wide range of values. (I tested i/10^d, for d in [1,6] and i in [1, 3e8], with both numerator and denominator rounded to integers, and never saw any extra digits)

For performance purposes I suspect we'd want to calculate up front the power of 10 that makes both tick0 and dtick into integers (which in itself is nontrivial if you want to cover weird cases like 1.001e-6 - but I'm sure you can figure it out 😅) and round it:
p = Math.round(Math.pow(10, digits))
then incrementing is just:
x1 = Math.round((x + delta) * p) / p

Now, the question is what cases that actually fixes? We could use it to fix cases we don't handle correctly right now in our own formatting - for example {tick0: 0.001, dtick: 1.001} which currently just rounds to 0, 1, 2... though there may be an easier way to fix that, just by improving the logic in the numeric case of autoTickRound to work more like the date case (this is the tick0/dtick logic I was thinking of earlier - funny that we do this for dates but not numbers currently!)

But it won't generally fix tickformat: 'p' or tickformat: 's' (or 'e', 'f', or 'g' for that matter). In both of these d3 internally may multiply a fraction by a power of 10 - and no matter how well we ensure the fraction has the correct string representation, we can't ensure the end product will be free of floating point rounding errors. Honestly I don't think there is a good solution for this - it's a problem inherent to floating point numbers, and anything we do to fix it will improperly round some cases where the user really wanted the extra precision.

Really the only solution I see is to encourage users never to use bare p or s, always include a length specifier. It's not a great solution, as it means the format can't change with zoom for example... unless we want to monkey with the d3 format language to let us insert an auto precision 😱

archmoj · 2019-07-30T16:22:26Z

@archmoj I did a little testing, and I think you may be on to something useful. AFAICT, as long as we add/subtract integers, and then divide by a power of 10 (again, explicitly rounded to be an integer), we're protected from these floating point errors over a quite wide range of values. (I tested i/10^d, for d in [1,6] and i in [1, 3e8], with both numerator and denominator rounded to integers, and never saw any extra digits)

@alexcjohnson Thanks very much for testing this.

archmoj · 2019-07-30T16:49:24Z

Now, the question is what cases that actually fixes? We could use it to fix cases we don't handle correctly right now in our own formatting - for example {tick0: 0.001, dtick: 1.001} which currently just rounds to 0, 1, 2... though there may be an easier way to fix that, just by improving the logic in the [numeric case of autoTickRound]

Good point. Most of these test would fail with the master version: https://github.com/plotly/plotly.js/blob/76b036859248c43518bd4f15f5dfa35444f665ed/test/jasmine/tests/lib_increment_numeric_test.js

@alexcjohnson Thank for mentioning 'e' & 'g' as well.
Having `{tick0: 0.001, dtick: 1.001, tickformat: 'e | g | p | s' } these are before & after codepens

etpinard · 2019-08-29T16:44:46Z

In order to potentially get this PR merged:

try finding papers about this. We can't be the first ones hitting this problem
try your tests on differents browsers (maybe using https://github.com/karma-runner/karma-sauce-launcher)
try the 55.00000000000000000 input using the latest d3-format. If still buggy, fill report over to d3

archmoj · 2019-09-11T19:54:33Z

try the 55.00000000000000000 input using the latest d3-format. If still buggy, fill report over to d3

This case is fixed in d3 > v3.

etpinard · 2019-12-03T22:57:06Z

@archmoj I stumbled upon https://github.com/MikeMcl/decimal.js/ - which might be of interest for this PR.

…licts

archmoj · 2020-09-01T04:42:15Z

Reworked in #5114.

archmoj added 3 commits July 22, 2019 10:49

add mock to test issue 3814

3623224

before fixing issue 3814

0a07168

fixing bug issue 3814 - use integers to increment aixs ticks

4855033

archmoj added bug something broken status: reviewable labels Jul 22, 2019

antoinerg reviewed Jul 22, 2019

View reviewed changes

add new test includes d3 p & s formats as well as dates and negative …

740574b

…numbers

antoinerg reviewed Jul 23, 2019

View reviewed changes

archmoj added 2 commits July 23, 2019 13:41

add incrementNumeric to Lib make it reusable and testable

916bf80

force magic number to be positive to avoid getting -0

added jasmine tests for incrementNumeric

86b135d

archmoj commented Jul 23, 2019

View reviewed changes

archmoj added the status: discussion needed label Jul 23, 2019

etpinard reviewed Jul 23, 2019

View reviewed changes

archmoj added 2 commits July 23, 2019 20:09

use scale instead of magic - only apply scale >= 1 - add more tests

ab2facf

improve on edge cases and test coverage

76b0368

archmoj added 2 commits March 14, 2020 22:08

Merge branch 'master' into fix3814-percent-formatting to resolve conf…

7f585ce

…licts

update header 2019 > 2020

14f704b

archmoj removed the status: discussion needed label Mar 17, 2020

archmoj added the status: on hold label Mar 17, 2020

archmoj mentioned this pull request Sep 1, 2020

Improve axis tick increment #5114

Merged

archmoj closed this Sep 1, 2020

archmoj deleted the fix3814-percent-formatting branch September 1, 2020 04:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve axis tick increment #4070

Improve axis tick increment #4070

archmoj commented Jul 22, 2019

antoinerg commented Jul 22, 2019

antoinerg commented Jul 22, 2019

archmoj commented Jul 22, 2019

archmoj commented Jul 22, 2019

antoinerg commented Jul 22, 2019

antoinerg commented Jul 22, 2019

antoinerg Jul 22, 2019

archmoj Jul 22, 2019

etpinard Jul 22, 2019

archmoj Jul 23, 2019

etpinard Jul 23, 2019

archmoj Jul 23, 2019

antoinerg Jul 23, 2019

archmoj Jul 23, 2019

antoinerg Jul 25, 2019

archmoj Jul 23, 2019

etpinard Jul 23, 2019

archmoj Jul 24, 2019

etpinard Jul 24, 2019

alexcjohnson commented Jul 30, 2019

archmoj commented Jul 30, 2019

archmoj commented Jul 30, 2019

etpinard commented Aug 29, 2019 •

edited by archmoj

Loading

archmoj commented Sep 11, 2019

etpinard commented Dec 3, 2019

archmoj commented Sep 1, 2020


		module.exports = function incrementNumeric(x, delta) {
		if(!delta) return x;

	function numFormat(v, ax, fmtoverride, hover) {
	var isNeg = v < 0;
	// max number of digits past decimal point to show
	var tickRound = ax._tickround;
	var exponentFormat = fmtoverride \|\| ax.exponentformat \|\| 'B';
	var exponent = ax._tickexponent;
	var tickformat = axes.getTickFormat(ax);
	var separatethousands = ax.separatethousands;

	// special case for hover: set exponent just for this value, and
	// add a couple more digits of precision over tick labels
	if(hover) {
	// make a dummy axis obj to get the auto rounding and exponent
	var ah = {
	exponentformat: exponentFormat,
	dtick: ax.showexponent === 'none' ? ax.dtick :
	(isNumeric(v) ? Math.abs(v) \|\| 1 : 1),
	// if not showing any exponents, don't change the exponent
	// from what we calculate
	range: ax.showexponent === 'none' ? ax.range.map(ax.r2d) : [0, v \|\| 1]
	};
	autoTickRound(ah);
	tickRound = (Number(ah._tickround) \|\| 0) + 4;
	exponent = ah._tickexponent;
	if(ax.hoverformat) tickformat = ax.hoverformat;
	}

	if(tickformat) return ax._numFormat(tickformat)(v).replace(/-/g, MINUS_SIGN);

	// 'epsilon' - rounding increment
	var e = Math.pow(10, -tickRound) / 2;

	// exponentFormat codes:
	// 'e' (1.2e+6, default)
	// 'E' (1.2E+6)
	// 'SI' (1.2M)
	// 'B' (same as SI except 10^9=B not G)
	// 'none' (1200000)
	// 'power' (1.2x10^6)
	// 'hide' (1.2, use 3rd argument=='hide' to eg
	// only show exponent on last tick)
	if(exponentFormat === 'none') exponent = 0;

	// take the sign out, put it back manually at the end
	// - makes cases easier
	v = Math.abs(v);
	if(v < e) {
	// 0 is just 0, but may get exponent if it's the last tick
	v = '0';
	isNeg = false;
	} else {
	v += e;
	// take out a common exponent, if any
	if(exponent) {
	v *= Math.pow(10, -exponent);
	tickRound += exponent;
	}
	// round the mantissa
	if(tickRound === 0) v = String(Math.floor(v));
	else if(tickRound < 0) {
	v = String(Math.round(v));
	v = v.substr(0, v.length + tickRound);
	for(var i = tickRound; i < 0; i++) v += '0';
	} else {
	v = String(v);
	var dp = v.indexOf('.') + 1;
	if(dp) v = v.substr(0, dp + tickRound).replace(/\.?0+$/, '');
	}
	// insert appropriate decimal point and thousands separator
	v = Lib.numSeparate(v, ax._separators, separatethousands);
	}

	// add exponent
	if(exponent && exponentFormat !== 'hide') {
	if(isSIFormat(exponentFormat) && beyondSI(exponent)) exponentFormat = 'power';

	var signedExponent;
	if(exponent < 0) signedExponent = MINUS_SIGN + -exponent;
	else if(exponentFormat !== 'power') signedExponent = '+' + exponent;
	else signedExponent = String(exponent);

	if(exponentFormat === 'e' \|\| exponentFormat === 'E') {
	v += exponentFormat + signedExponent;
	} else if(exponentFormat === 'power') {
	v += '×10<sup>' + signedExponent + '</sup>';
	} else if(exponentFormat === 'B' && exponent === 9) {
	v += 'B';
	} else if(isSIFormat(exponentFormat)) {
	v += SIPREFIXES[exponent / 3 + 5];
	}
	}

	// put sign back in and return
	// replace standard minus character (which is technically a hyphen)
	// with a true minus sign
	if(isNeg) return MINUS_SIGN + v;
	return v;
	}

Improve axis tick increment #4070

Improve axis tick increment #4070

Conversation

archmoj commented Jul 22, 2019

antoinerg commented Jul 22, 2019

antoinerg commented Jul 22, 2019

archmoj commented Jul 22, 2019

archmoj commented Jul 22, 2019

antoinerg commented Jul 22, 2019

antoinerg commented Jul 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcjohnson commented Jul 30, 2019

archmoj commented Jul 30, 2019

archmoj commented Jul 30, 2019

etpinard commented Aug 29, 2019 • edited by archmoj Loading

archmoj commented Sep 11, 2019

etpinard commented Dec 3, 2019

archmoj commented Sep 1, 2020

etpinard commented Aug 29, 2019 •

edited by archmoj

Loading