quantize icon indicating copy to clipboard operation
quantize copied to clipboard

Correct the number of colors returned by quantize

Open ijambro opened this issue 4 years ago • 5 comments

Closes #9 The number of colors returned by quantize should match maxcolors, including for values > 7.

I've tested this fix using a test.js from a separate project that requires quantize, for many values ranging from 1 to 256, and this fix corrects the off-by-one color count.

A test.js and a Readme update would help to support this Pull Request. Let me know if you're interested.

Curious if anyone else has noticed or investigated this behavior?

ijambro avatar Apr 22 '20 05:04 ijambro

I created Mocha tests for the quantize function, asserting that the format of all returned palettes is valid, for a variety of inputs.

For each of the tests below, I run it using many values of maximumColorCount between 2 and 256:

✓ works on 1-pixel 1-color images
✓ works on 5-pixel 1-color images
✓ works on 1000-pixel 1-color images
✓ works on 5-pixel 5-color images
✓ works on 1000-pixel 5-color images
✓ works on 20-pixel 20-color images
✓ works on 1000-pixel 20-color images

The most interesting results are how the returned ("Actual") count of colors lines up with the requested ("Expected") maximum color count.

Using the current quantize code: 1-pixel: always returns 1 color More than 1 pixel: Actual: 3, Expected: 2 Actual: 4, Expected: 3 Actual: 4, Expected: 4 Actual: 5, Expected: 5 Actual: 6, Expected: 6 Actual: 7, Expected: 7 Actual: 7, Expected: 8 Actual: 8, Expected: 9 Actual: 9, Expected: 10 Actual: 10, Expected: 11 Actual: 11, Expected: 12 Actual: 12, Expected: 13 Actual: 13, Expected: 14 Actual: 14, Expected: 15 Actual: 15, Expected: 16 Actual: 31, Expected: 32 Actual: 63, Expected: 64 Actual: 127, Expected: 128 Actual: 255, Expected: 256

Using my proposed change, adding "+ 1" to current quantize code line 474: 1-pixel: always returns 1 color More than 1 pixel: Actual: 3, Expected: 2 Actual: 4, Expected: 3 Actual: 4, Expected: 4 Actual: 5, Expected: 5 Actual: 6, Expected: 6 Actual: 7, Expected: 7 Actual: 8, Expected: 8 Actual: 9, Expected: 9 Actual: 10, Expected: 10 Actual: 11, Expected: 11 Actual: 12, Expected: 12 Actual: 13, Expected: 13 Actual: 14, Expected: 14 Actual: 15, Expected: 15 Actual: 16, Expected: 16 Actual: 32, Expected: 32 Actual: 64, Expected: 64 Actual: 128, Expected: 128 Actual: 256, Expected: 256

Two conclusions:

  1. For requested ("Expected") maximum color count of 2 and 3, the response includes one too many - exceeding the maximum - in both the original code and my change.
  2. For requested ("Expected") maximum color count greater than 7, my change corrects the issue and responds with the exact amount requested.

ijambro avatar May 17 '20 05:05 ijambro

After testing AlfredJKwack's change from #6 , I've seen that it returns the exact count of colors requested for all input values between 2 and 256! I've replaced my partial correction with his superior fix.

image

image

ijambro avatar May 25 '20 05:05 ijambro

// @olivierlesnicki

Richienb avatar Aug 05 '20 06:08 Richienb

+1 for this!

alexharris avatar Dec 18 '23 00:12 alexharris

and, if anyone else stumbles across this I have a fork with this pull request, which seems to fix the issue, implemented here:

https://github.com/alexharris/quantize

alexharris avatar Dec 18 '23 05:12 alexharris