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

“.getDeltaE94” returns a “NaN”. ……How come? #9

Closed
issuefiler opened this issue Jan 10, 2020 · 4 comments
Closed

“.getDeltaE94” returns a “NaN”. ……How come? #9

issuefiler opened this issue Jan 10, 2020 · 4 comments

Comments

@issuefiler
Copy link

issuefiler commented Jan 10, 2020

Alright, I just stumbled upon a very specific input that caused a mysterious crash.

const ΔE = require("delta-e");
const distance = ΔE.getDeltaE94(
	{
		L: 53.23288178584245,
		A: 80.10930952982204,
		B: 67.22006831026425
	},
	{
		L: 50.9588099835815,
		A: 77.47798295202801,
		B: 65.01211079141827
	}
);
console.log(distance); // NaN

This ends up returning a NaN. I believe nothing’s special about those numbers? What makes it so special that it crashes my application?

@issuefiler issuefiler changed the title “.getDeltaE94” returns “NaN”. ……How come? “.getDeltaE94” returns a “NaN”. ……How come? Jan 10, 2020
@zschuessler
Copy link
Owner

I took a brief look, the A calculation specifically is causing the issue:

https://github.com/zschuessler/DeltaE/blob/master/src/dE94.js#L79

When I clamp the number of your colors by a tenths position it works correctly. I would assume a rounding problem. If you find out let me know, if not I'll come back to this when I have free time.

@issuefiler
Copy link
Author

Test

I did a quick logging test on my browser.

function dE94(x1, x2, weights) {
    this.x1 = x1;
    this.x2 = x2;
    
    this.weights = weights || {};
    this.weights.lightness = this.weights.lightness || 1;
    this.weights.chroma = this.weights.chroma || 1;
    this.weights.hue = this.weights.hue || 1;
    
    if (1 === this.weights.lightness) {
        this.weights.K1 = 0.045;
        this.weights.K2 = 0.015;
    } else {
        this.weights.K1 = 0.048;
        this.weights.K2 = 0.014;
    }
}

/**
 * Returns the dE94 value.
 * @method
 * @returns {number}
 */
dE94.prototype.getDeltaE = function() {
    var x1 = this.x1;
    var x2 = this.x2;
    var sqrt = Math.sqrt;
    var pow = Math.pow;
    console.log(this.calculateL(x1, x2), this.calculateA(x1, x2), this.calculateB(x1, x2));
    return sqrt(
        pow(this.calculateL(x1, x2), 2) +
        pow(this.calculateA(x1, x2), 2) +
        pow(this.calculateB(x1, x2), 2)
    );
};

/**
 * Calculates the lightness value.
 * @method
 * @returns {number}
 */
dE94.prototype.calculateL = function(x1, x2) {
    return (x1.L - x2.L) / this.weights.lightness;
};

/**
 * Calculates the chroma value.
 * @method
 * @returns {number}
 */
dE94.prototype.calculateA = function(x1, x2) {
    var sqrt = Math.sqrt;
    var pow = Math.pow;
    
    //top
    var c1 = sqrt(pow(x1.A, 2) + pow(x1.B, 2));
    var c2 = sqrt(pow(x2.A, 2) + pow(x2.B, 2));
    var cab = c1 - c2;
    
    // bottom
    var sc = 1 + (this.weights.K1 * c1);
    
    return cab / (this.weights.chroma * sc);
};

/**
 * Calculates the hue value.
 * @method
 * @returns {number}
 */
dE94.prototype.calculateB = function(x1, x2) {
    var sqrt = Math.sqrt;
    var pow = Math.pow;
    
    // cab
    var c1 = sqrt(pow(x1.A, 2) + pow(x1.B, 2));
    var c2 = sqrt(pow(x2.A, 2) + pow(x2.B, 2));
    var cab = c1 - c2;
    
    // top
    var a = x1.A - x2.A;
    var b = x1.B - x2.B;
    var hab = sqrt(
        pow(a, 2) +
        pow(b, 2) -
        pow(cab, 2)
    );
console.log("HAB", pow(a, 2) +
        pow(b, 2) -
        pow(cab, 2), hab, pow(a, 2) +
        pow(b, 2),
        pow(cab, 2));
    
    // bottom
    var c1 = sqrt(pow(x1.A, 2) + pow(x1.B, 2));
    var sh = 1 + (this.weights.K2 * c1);
    console.log(hab, sh);
    return hab / sh;
};
(new dE94({
		L: 53.23288178584245,
		A: 80.10930952982204,
		B: 67.22006831026425
	},
	{
		L: 50.9588099835815,
		A: 77.47798295202801,
		B: 65.01211079141827
	})).getDeltaE()
HAB -6.394884621840902e-14 NaN 11.798955964033755 11.79895596403382
NaN 2.568632776599043
2.2740718022609485 0.6020017603282479 NaN
HAB -6.394884621840902e-14 NaN 11.798955964033755 11.79895596403382
NaN 2.568632776599043
NaN

Problem

And it turned out to be due to floating-point errors in the calculation. What was supposed to be 0 couldn’t but -0.00000000000006394884621840902, which caused Math.sqrt to return a NaN.


Solution

I suggest an error-tolerant square-rooting static method value => (Math.sqrt(value) || 0).

The visualized problem

@zschuessler
Copy link
Owner

Wow, thanks for doing that. Great work!

I'm in the process of moving across the country so I don't have much time to review this for a couple weeks. I'll take a look as soon as I can though!

@zschuessler
Copy link
Owner

Thanks again for the detailed explanation, that was helpful! I pushed an update to handle the sqrt issue in eE94 and also added a Mocha test to test for the issue. New release in 0.0.8 is live.

I also toyed around with using a couple workarounds to make floating-point safe calculations, but all of the options were convoluted and impacted performance. I'm not opposed to addressing that again but it seems out of scope for the common use case of this package.

Btw: the move across the country went great. The east coast is wonderful 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants