Skip to content

Add support for digits keyword argument of round() #235

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
Oct 16, 2020

Conversation

kimikage
Copy link
Collaborator

@kimikage kimikage commented Oct 2, 2020

This only supports the non-negative digits option. All other options throw an error.
This provides specialized implementations only for N0f8 and Fixed{Int8}. The other types use fallback with floating-point operations and therefore it is slower and less accurate.

Fixes #234

This only supports the non-negative `digits` option.
All other options throw an error.
This provides specialized implementations only for `N0f8` and `Fixed{Int8}`.
The other types use fallback with floating-point operations and therefore it is slower and less accurate.
@codecov
Copy link

codecov bot commented Oct 2, 2020

Codecov Report

Merging #235 into master will increase coverage by 0.23%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #235      +/-   ##
==========================================
+ Coverage   96.24%   96.47%   +0.23%     
==========================================
  Files           6        6              
  Lines         692      738      +46     
==========================================
+ Hits          666      712      +46     
  Misses         26       26              
Impacted Files Coverage Δ
src/FixedPointNumbers.jl 96.69% <100.00%> (+0.20%) ⬆️
src/fixed.jl 98.11% <100.00%> (+0.19%) ⬆️
src/normed.jl 95.94% <100.00%> (+0.25%) ⬆️

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 f57e96b...23aa8e5. Read the comment docs.

@kimikage
Copy link
Collaborator Author

kimikage commented Oct 2, 2020

Benchmark

using ColorTypes, BenchmarkTools
image = rand(RGB{N0f8}, 1000, 1000);

function quantize(x::N0f8)
    floor(x, digits=1)
end
function quantize(color::AbstractRGB)
    mapc(quantize, color)
end


function quantize_f(x::N0f8)
    floor(float(x), digits=1) % N0f8
end
function quantize_f(color::AbstractRGB)
    mapc(quantize_f, color)
end
julia> @benchmark quantize.($image)
BenchmarkTools.Trial:
  memory estimate:  2.86 MiB
  allocs estimate:  2
  --------------
  minimum time:     1.504 ms (0.00% GC)
  median time:      1.532 ms (0.00% GC)
  mean time:        1.731 ms (10.89% GC)
  maximum time:     5.461 ms (71.47% GC)
  --------------
  samples:          2888
  evals/sample:     1

julia> @benchmark quantize_f.($image)
BenchmarkTools.Trial:
  memory estimate:  2.86 MiB
  allocs estimate:  2
  --------------
  minimum time:     18.977 ms (0.00% GC)
  median time:      19.034 ms (0.00% GC)
  mean time:        19.298 ms (1.17% GC)
  maximum time:     22.876 ms (16.72% GC)
  --------------
  samples:          260
  evals/sample:     1
n4f4  = rand(N4f4,  1000);
n0f8  = rand(N0f8,  1000);
n0f16 = rand(N0f16, 1000);

q3f4  = rand(Q3f4,  1000);
q0f7  = rand(Q0f7,  1000);
q0f15 = rand(Q0f15, 1000);
julia> @btime trunc.($n4f4,  digits=1); # not optimized
  13.199 μs (1 allocation: 1.06 KiB)

julia> @btime trunc.($n0f8,  digits=1);
  161.658 ns (1 allocation: 1.06 KiB)

julia> @btime trunc.($n0f8,  digits=2);
  158.163 ns (1 allocation: 1.06 KiB)

julia> @btime trunc.($n0f16, digits=2); # not optimized
  15.100 μs (1 allocation: 2.13 KiB)

julia> @btime trunc.($q3f4,  digits=1);
  221.577 ns (1 allocation: 1.06 KiB)

julia> @btime trunc.($q0f7,  digits=1);
  126.801 ns (1 allocation: 1.06 KiB)

julia> @btime trunc.($q0f7,  digits=2);
  222.530 ns (1 allocation: 1.06 KiB)

julia> @btime trunc.($q0f15, digits=2); # not optimized
  5.767 μs (1 allocation: 2.13 KiB)

@kimikage
Copy link
Collaborator Author

This changes the behavior as a result, but I think this is rather a bug fix. So, I will merge this.

@kimikage kimikage merged commit 6236e66 into JuliaMath:master Oct 16, 2020
@kimikage kimikage deleted the round_kwargs branch October 16, 2020 11:56
@kimikage kimikage added the breaking Major breaking change label Mar 3, 2021
@kimikage kimikage mentioned this pull request Apr 30, 2024
38 tasks
kimikage added a commit to kimikage/FixedPointNumbers.jl that referenced this pull request May 1, 2024
kimikage added a commit to kimikage/FixedPointNumbers.jl that referenced this pull request May 1, 2024
kimikage added a commit to kimikage/FixedPointNumbers.jl that referenced this pull request May 1, 2024
kimikage added a commit to kimikage/FixedPointNumbers.jl that referenced this pull request May 1, 2024
kimikage added a commit to kimikage/FixedPointNumbers.jl that referenced this pull request May 1, 2024
kimikage added a commit to kimikage/FixedPointNumbers.jl that referenced this pull request May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Major breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Follow contract of floor/ceil
1 participant