add option 'threshold' representing % of different pixels (number between 0 and 100) #15

nikchosh · 2015-03-29T19:58:38Z

Threshold should be a value between 0 and 100 (% of pixels that are
different). Should take into account that compare.exe sometimes returns normalized numbers of kind 9.9999e-999)

Threshold should be a value between 0 and 1 (% of pixels that are different). DiffPath is the path and name of the diff image. Useful, if user wants to remove identical diff images.

twolfson · 2015-03-29T20:43:10Z

lib/image-diff.js

@@ -73,7 +73,7 @@ ImageDiff.createDiff = function (options, cb) {
    // DEV: According to http://www.imagemagick.org/discourse-server/viewtopic.php?f=1&t=17284
    // DEV: These values are the total square root mean square (RMSE) pixel difference across all pixels and its percentage
    // TODO: This is not very multi-lengual =(
-    var resultInfo = stderr.match(/all: (\d+\.?\d*) \((\d+\.?\d*)\)/);
+    var resultInfo = stderr.match(/all: (\d+\.?\d*) \((.*?)\)/);


We were intentionally keeping the regexp sniffing digits to keep it as efficient as possible. Please move back to the previous version.

I remember why I changed this - in case of only a few pixel difference, we get a result like this from compare.exe:
Channel distortion: RMSE
red: 0.00129297 (5.07046e-006)
green: 0 (0)
blue: 0.00158356 (6.21002e-006)
all: 0.00118031 (4.62868e-006)
So, we can either use (. * ) or /\d+.?\d * (e-\d+)?/ to allow for something small

Ah, interesting. We should probably isolate the parser to a separate function so we can test these individually. I imagine it will become very tedious to create a set of separate images for each of these cases.

As for the regexp, being explicit is always more useful than being implicit. Additionally, we should probably include examples of the different values in comments. The regexp should look like:

/all: (\d+\.?\d*) \((\d+\.?\d*e?\-?\d*)\)/)

This allows for 0, 0.1, 0.1e-3, and even unexpected cases like 1e3, 1.0e3.

twolfson · 2015-03-29T20:47:16Z

The gist is here but we need tests. Can you add some?

nikchosh · 2015-03-29T21:43:35Z

I have included all your suggestions (ultra new to git -- sorry to make you my mentor :) ) I need to look at the tests. I am tester, and getting feet wet in Node development

twolfson · 2015-03-29T23:05:38Z

I can't see your changes. Please push them to update the PR (pull request).

The values of threshold must be between 0 and 100 and represents percentage of different pixels.

twolfson · 2015-03-30T01:15:37Z

lib/image-diff.js

-    var totalDifference = resultInfo[1];
-    return cb(null, totalDifference === '0');
+    var percentDifference = parseFloat(resultInfo[2], 10);
+    var threshold = (typeof(options.threshold) === 'undefined' || isNaN(parseFloat(options.threshold, 10))) ? 0.0 : parseFloat(options.threshold);


There is a lot going on in this line.

Using typeof to detect undefined is only necessary when looking at a global (e.g. `typeof window === 'undefined')

Otherwise, we can use a simpler direct comparison with undefined (e.g. options.foo === undefined)

We don't need to detect undefined, a fallback will work fine (e.g. var foo = options.bar || baz;)

We don't need to be defensive and coerce options.threshold. We can assume the developer wants the library to work and will pass in a Number (i.e. no isNaN/parseFloat necessary)

In JS, all Numbers are 64 bit floats. The decimal isn't necessary; 0 will work fine.

https://developer.mozilla.org/en-US/docs/Glossary/Number

The simplified result should be:

var threshold = options.threshold || 0;

twolfson · 2015-03-30T01:17:14Z

Getting closer, still needs tests though =/

nikchosh · 2015-03-30T23:54:00Z

Tests added - probably documentation needs to be updated too

twolfson · 2015-03-31T05:50:49Z

test/image-diff_test.js

@@ -31,7 +31,7 @@ describe('image-diff', function () {
    runImageDiff({
      actualImage: __dirname + '/test-files/checkerboard.png',
      expectedImage: __dirname + '/test-files/white.png',
-      diffImage: __dirname + '/actual-files/different.png'
+      diffImage: __dirname + '/actual-files/different.png',


Can you move back this line? It is noise in the PR =/

twolfson · 2015-03-31T06:01:36Z

Everything looks fine. Mostly fine tuning things now.

nikchosh · 2015-03-31T21:06:57Z

Taken all your comments into account - please review

twolfson · 2015-04-01T04:23:03Z

There seem to still be a few lingering inline comments that I would like to get sorted out. Can we take care of those?

nikchosh · 2015-04-06T22:04:49Z

Would you like me to remove all // comments DEV: etc.

twolfson · 2015-04-07T06:41:56Z

Nope, I am referring to these:

https://github.com/uber/image-diff/pull/15/files#r27454478

https://github.com/uber/image-diff/pull/15/files#r27454735

https://github.com/uber/image-diff/pull/15/files#r27454747

nikchosh · 2015-04-20T04:13:34Z

Hi Todd, any way to merge the changes? :)

twolfson · 2015-04-20T06:13:05Z

There is still this trailing comma comment left =/

https://github.com/uber/image-diff/pull/15/files#r27454478

Additionally, we seem to be blocked by #16. I will bump the issue so it can be addressed.

nikchosh · 2015-04-20T20:07:43Z

Commas fixed

twolfson · 2015-04-22T03:13:40Z

Sweet, everything looks great now. Still waiting on a reply in #16

mlmorg · 2015-04-25T19:18:06Z

Can we rebase and squash into a single commit? Thanks!

twolfson · 2016-05-11T19:02:24Z

This PR hasn't been active in over 1 year. We can probably close it due to inactivity

add option 'threshold', add return value 'diffPath'

0652fb7

Threshold should be a value between 0 and 1 (% of pixels that are different). DiffPath is the path and name of the diff image. Useful, if user wants to remove identical diff images.

twolfson reviewed Mar 29, 2015
View reviewed changes

add 'threshold' option for comparison

f282385

The values of threshold must be between 0 and 100 and represents percentage of different pixels.

nikchosh changed the title ~~add option 'threshold', add return value 'diffPath'~~ add option 'threshold' representing % of different pixels (number between 0 and 100) Mar 30, 2015

twolfson reviewed Mar 30, 2015
View reviewed changes

Nick Georgiev added 2 commits March 30, 2015 15:10

simplifying check for threshold option existence

d691a4e

add tests for new option 'threshold'

6261427

twolfson reviewed Mar 31, 2015
View reviewed changes

fine tune tests

9db762f

Nick Georgiev added 2 commits April 8, 2015 09:16

rearrange tests

b517c85

add timeout to tests

1a0b2d7

twolfson mentioned this pull request Apr 20, 2015

Determine release workflow #16

Closed

remove trailing commas

350fdea

mlmorg closed this May 24, 2016

twolfson mentioned this pull request Dec 9, 2016

Is there any way to test a threshold? #14

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add option 'threshold' representing % of different pixels (number between 0 and 100) #15

add option 'threshold' representing % of different pixels (number between 0 and 100) #15

nikchosh commented Mar 29, 2015

twolfson Mar 29, 2015

nikchosh Mar 29, 2015

twolfson Mar 29, 2015

twolfson commented Mar 29, 2015

nikchosh commented Mar 29, 2015

twolfson commented Mar 29, 2015

twolfson Mar 30, 2015

twolfson commented Mar 30, 2015

nikchosh commented Mar 30, 2015

twolfson Mar 31, 2015

twolfson commented Mar 31, 2015

nikchosh commented Mar 31, 2015

twolfson commented Apr 1, 2015

nikchosh commented Apr 6, 2015

twolfson commented Apr 7, 2015

nikchosh commented Apr 20, 2015

twolfson commented Apr 20, 2015

nikchosh commented Apr 20, 2015

twolfson commented Apr 22, 2015

mlmorg commented Apr 25, 2015

twolfson commented May 11, 2016

add option 'threshold' representing % of different pixels (number between 0 and 100) #15

add option 'threshold' representing % of different pixels (number between 0 and 100) #15

Conversation

nikchosh commented Mar 29, 2015

twolfson Mar 29, 2015

Choose a reason for hiding this comment

nikchosh Mar 29, 2015

Choose a reason for hiding this comment

twolfson Mar 29, 2015

Choose a reason for hiding this comment

twolfson commented Mar 29, 2015

nikchosh commented Mar 29, 2015

twolfson commented Mar 29, 2015

twolfson Mar 30, 2015

Choose a reason for hiding this comment

twolfson commented Mar 30, 2015

nikchosh commented Mar 30, 2015

twolfson Mar 31, 2015

Choose a reason for hiding this comment

twolfson commented Mar 31, 2015

nikchosh commented Mar 31, 2015

twolfson commented Apr 1, 2015

nikchosh commented Apr 6, 2015

twolfson commented Apr 7, 2015

nikchosh commented Apr 20, 2015

twolfson commented Apr 20, 2015

nikchosh commented Apr 20, 2015

twolfson commented Apr 22, 2015

mlmorg commented Apr 25, 2015

twolfson commented May 11, 2016