-
-
Notifications
You must be signed in to change notification settings - Fork 888
Use more accuracy when calculating variance. Fix #866 #874
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #874 +/- ##
=========================================
Coverage ? 88.96%
=========================================
Files ? 1018
Lines ? 44578
Branches ? 3235
=========================================
Hits ? 39661
Misses ? 4196
Partials ? 721
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we introduce a test that fails with the old code and is passing using the updated one? Most straightforward: PNG test based on the image in #866. (Not sure if it's the best.)
|
I've added the unit tests from the original implementation. |
|
I will check again as soon as I get home. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JimBobSquarePants I re-executed the tests after replacing the WuFrameQuantizer<T> implementation with the code before the change, and all tests passed. This means that the improvement introduced by this PR is not covered!
I think there is an easy-to-code, implementation-agnostic approach to test IQuantizer implementations:
- Create an
Image<T>fromQuantizedFrame<T>representing the quantization result - Compare the quantized image against the original one using a tolerance
With this approach the old code should lead to a test failure using the image from #866!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 colors before VS 48 colors after. Looks pretty good. Great that the you also added the QuantizedFrame<T> refactor. Let's merge this in!
…bors#874) * Use more accuracy when calculating variance. Fix SixLabors#866 * Add unit tests * Add test that fails with old image. * Make IFrameQuantizer IDisposable * Update GifEncoderCore.cs
Prerequisites
Description
Use double precision to calculate variance. This prevents early escape from color count calculation. Fix #866
Also cleaned up a little.