Skip to content

Fix overflow errors in division by Normed/Fixed #168

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

Merged
merged 1 commit into from
May 8, 2021

Conversation

kimikage
Copy link
Collaborator

@kimikage kimikage commented May 3, 2021

This also removes some of the optimization using the reciprocals.

julia> Gray(reinterpret(N0f8, 0x5)) / reinterpret(N0f8, 0x6)
Gray{N0f8}(0.831)     # this PR
ERROR: ArgumentError: # v0.9.4

julia> RGB(reinterpret(N0f8, 0x5)) / reinterpret(N0f8, 0x6)
RGB{N0f8}(0.831,0.831,0.831) # this PR
RGB{N0f8}(0.835,0.835,0.835) # v0.9.4

julia> Gray(eps(N0f8)) / 3.0f0
Gray{Float32}(0.0013071896f0) # this PR
Gray{Float32}(0.0013071897f0) # v0.9.4

In the case of RGB{N0f8}, the absolute error does not change, but the round-to-even is applied in this PR.

Fixes #154

@codecov
Copy link

codecov bot commented May 3, 2021

Codecov Report

Merging #168 (74c816b) into master (efa9959) will increase coverage by 0.28%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #168      +/-   ##
==========================================
+ Coverage   84.68%   84.97%   +0.28%     
==========================================
  Files           2        2              
  Lines         209      213       +4     
==========================================
+ Hits          177      181       +4     
  Misses         32       32              
Impacted Files Coverage Δ
src/ColorVectorSpace.jl 90.04% <100.00%> (+0.20%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update efa9959...74c816b. Read the comment docs.

@kimikage
Copy link
Collaborator Author

kimikage commented May 3, 2021

Benchmarks

Script
using ColorVectorSpace, ColorTypes, FixedPointNumbers, BenchmarkTools

BenchmarkTools.DEFAULT_PARAMETERS.seconds = 10
N = 1000

g_n0f8  = rand(Gray{N0f8}, N, N);
g_n0f16 = rand(Gray{N0f16}, N, N);
g_f32   = rand(Gray{Float32}, N, N);
g_f64   = rand(Gray{Float64}, N, N);

r_n0f8  = fill(1N0f8, N, N);
r_f64   = fill(1.0, N, N);
r_int   = fill(1, N, N);

rgb_n0f8  = rand(RGB{N0f8}, N, N);
rgb_n0f16 = rand(RGB{N0f16}, N, N);
rgb_f32   = rand(RGB{Float32}, N, N);
rgb_f64   = rand(RGB{Float64}, N, N);

for r in (1N0f8, 1.0, 1)
    @show r
    @btime $g_n0f8  ./ $r;
    @btime $g_n0f16 ./ $r;
    @btime $g_f32   ./ $r;
    @btime $g_f64   ./ $r;
end

for r in (r_n0f8, r_f64, r_int)
    @show typeof(r)
    @btime $g_n0f8  ./ $r;
    @btime $g_n0f16 ./ $r;
    @btime $g_f32   ./ $r;
    @btime $g_f64   ./ $r;
end

for r in (1N0f8, 1.0, 1)
    @show r
    @btime $rgb_n0f8  ./ $r;
    @btime $rgb_n0f16 ./ $r;
    @btime $rgb_f32   ./ $r;
    @btime $rgb_f64   ./ $r;
end

for r in (r_n0f8, r_f64, r_int)
    @show typeof(r)
    @btime $rgb_n0f8  ./ $r;
    @btime $rgb_n0f16 ./ $r;
    @btime $rgb_f32   ./ $r;
    @btime $rgb_f64   ./ $r;
end
julia> versioninfo()
Julia Version 1.6.1
Commit 6aaedecc44 (2021-04-23 05:59 UTC)
Platform Info:
  OS: Windows (x86_64-w64-mingw32)
  CPU: Intel(R) Core(TM) i7-8565U CPU @ 1.80GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-11.0.1 (ORCJIT, skylake)
operation v0.9.4 before after
g_n0f8 ./ 1N0f8 ( 9.345 ms) (0.069 ms) 1.505 ms
g_n0f16 ./ 1N0f8 ( 9.938 ms) (0.312 ms) 1.703 ms
g_f32 ./ 1N0f8 ( 0.640 ms) (0.630 ms) 0.615 ms
g_f64 ./ 1N0f8 ( 1.437 ms) (1.472 ms) 1.464 ms
g_n0f8 ./ 1.0 1.152 ms 1.136 ms 1.165 ms
g_n0f16 ./ 1.0 1.196 ms 1.188 ms 1.236 ms
g_f32 ./ 1.0 1.257 ms 1.233 ms 1.264 ms
g_f64 ./ 1.0 1.418 ms 1.442 ms 1.474 ms
g_n0f8 ./ 1 0.592 ms 0.577 ms 0.586 ms
g_n0f16 ./ 1 0.598 ms 0.609 ms 0.604 ms
g_f32 ./ 1 0.616 ms 0.662 ms 0.652 ms
g_f64 ./ 1 1.437 ms 1.480 ms 1.431 ms
g_n0f8 ./ r_n0f8 ( 9.859 ms) (3.105 ms) 1.689 ms ✔️
g_n0f16 ./ r_n0f8 (10.336 ms) (3.825 ms) 2.034 ms ✔️
g_f32 ./ r_n0f8 ( 3.492 ms) (3.508 ms) 0.623 ms ✔️
g_f64 ./ r_n0f8 ( 4.190 ms) (4.055 ms) 1.444 ms ✔️
g_n0f8 ./ r_f64 1.520 ms 1.505 ms 1.470 ms
g_n0f16 ./ r_f64 1.514 ms 1.533 ms 1.607 ms
g_f32 ./ r_f64 1.632 ms 1.655 ms 1.613 ms
g_f64 ./ r_f64 1.714 ms 1.768 ms 1.794 ms
g_n0f8 ./ r_int 1.384 ms 1.403 ms 1.428 ms
g_n0f16 ./ r_int 1.435 ms 1.381 ms 1.464 ms
g_f32 ./ r_int 1.324 ms 1.380 ms 1.393 ms
g_f64 ./ r_int 1.827 ms 1.851 ms 1.878 ms
operation v0.9.4 before after
rgb_n0f8 ./ 1N0f8 9.660 ms 9.840 ms 14.702 ms
rgb_n0f16 ./ 1N0f8 10.225 ms 9.997 ms 14.718 ms
rgb_f32 ./ 1N0f8 (14.172 ms) (13.965 ms) 2.678 ms ✔️
rgb_f64 ./ 1N0f8 (15.899 ms) (16.272 ms) 5.213 ms ✔️
rgb_n0f8 ./ 1.0 3.711 ms 3.756 ms 3.688 ms
rgb_n0f16 ./ 1.0 3.932 ms 3.933 ms 3.829 ms
rgb_f32 ./ 1.0 4.144 ms 4.197 ms 4.298 ms
rgb_f64 ./ 1.0 4.581 ms 4.711 ms 4.747 ms
rgb_n0f8 ./ 1 2.137 ms 2.078 ms 2.015 ms
rgb_n0f16 ./ 1 2.697 ms 2.676 ms 2.222 ms
rgb_f32 ./ 1 2.533 ms 2.516 ms 2.532 ms
rgb_f64 ./ 1 4.611 ms 4.714 ms 4.641 ms
rgb_n0f8 ./ r_n0f8 9.954 ms 10.004 ms 14.264 ms
rgb_n0f16 ./ r_n0f8 10.561 ms 10.454 ms 14.975 ms
rgb_f32 ./ r_n0f8 (14.058 ms) (13.961 ms) 2.709 ms ✔️
rgb_f64 ./ r_n0f8 (15.494 ms) (16.034 ms) 4.869 ms ✔️
rgb_n0f8 ./ r_f64 4.067 ms 4.006 ms 4.031 ms
rgb_n0f16 ./ r_f64 4.262 ms 3.958 ms 4.172 ms
rgb_f32 ./ r_f64 4.415 ms 4.383 ms 4.330 ms
rgb_f64 ./ r_f64 4.992 ms 4.865 ms 5.054 ms
rgb_n0f8 ./ r_int 2.850 ms 2.590 ms 2.594 ms
rgb_n0f16 ./ r_int 3.458 ms 3.347 ms 3.103 ms
rgb_f32 ./ r_int 2.873 ms 2.568 ms 2.659 ms
rgb_f64 ./ r_int 5.059 ms 4.649 ms 4.585 ms

@kimikage
Copy link
Collaborator Author

kimikage commented May 3, 2021

As shown above, this will result in slowdowns in some cases, but if the speed performance is really critical, I don't think you should use the "checked" division.

@kimikage kimikage marked this pull request as ready for review May 3, 2021 15:32
This also removes some of the optimization using the reciprocals.
@kimikage
Copy link
Collaborator Author

kimikage commented May 4, 2021

If we find no issues other than slowdowns in the edge cases, I will merge this in this weekend. Then, I'll update PR #153 and release v0.9.5 in the next week.

@kimikage
Copy link
Collaborator Author

kimikage commented May 8, 2021

Since this is intended to fix a bug, I'll merge this.
If you have problems with slowdowns, please report them. In general, lookup tables are effective for N0f8 calculations.

@kimikage kimikage merged commit 32373c7 into JuliaGraphics:master May 8, 2021
@kimikage kimikage deleted the issue154 branch May 8, 2021 03:14
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

Successfully merging this pull request may close these issues.

Avoidable overflow errors in division
1 participant