Skip to content
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

Add checked arithmetic for / #222

Merged
merged 1 commit into from
Aug 31, 2020
Merged

Conversation

kimikage
Copy link
Collaborator

@kimikage kimikage commented Aug 25, 2020

This also changes the implementation of / for Fixed. The new / also checks for overflow.

/ (checked_fdiv) throws DivideError for division by zero and OverflowError for overflow.

julia> 0.5Q0f7 / 0Q0f7 # v0.8.4 and this PR
ERROR: DivideError: integer division error

julia> 0.5N0f8 / 0N0f8 # v0.8.4
ERROR: ArgumentError: Normed{UInt8,8} is an 8-bit type representing 256 values from 0.0 to 1.0; cannot represent Inf

julia> 0.5N0f8 / 0N0f8 # this PR
ERROR: DivideError: integer division error
julia> -1Q0f7 / -1Q0f7 # v0.8.4
-1.0Q0f7

julia> -1Q0f7 / -1Q0f7 # this PR
ERROR: OverflowError: -1.0Q0f7 / -1.0Q0f7 overflowed for type Q0f7

julia> 1N0f8 / 0.5N0f8 # v0.8.4
ERROR: ArgumentError: Normed{UInt8,8} is an 8-bit type representing 256 values from 0.0 to 1.0; cannot represent 1.9921874

julia> 1N0f8 / 0.5N0f8 # this PR
ERROR: OverflowError: 1.0N0f8 / 0.502N0f8 overflowed for type N0f8

@kimikage kimikage force-pushed the checked_fdiv branch 2 times, most recently from 1e1bcab to b04f088 Compare August 29, 2020 04:17
@kimikage kimikage added the breaking Major breaking change label Aug 29, 2020
@kimikage kimikage force-pushed the checked_fdiv branch 2 times, most recently from c8c3fd4 to 416e425 Compare August 29, 2020 05:09
@codecov
Copy link

codecov bot commented Aug 29, 2020

Codecov Report

Merging #222 into master will increase coverage by 0.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #222      +/-   ##
==========================================
+ Coverage   91.46%   91.58%   +0.11%     
==========================================
  Files           6        6              
  Lines         574      582       +8     
==========================================
+ Hits          525      533       +8     
  Misses         49       49              
Impacted Files Coverage Δ
src/fixed.jl 95.45% <ø> (-0.04%) ⬇️
src/normed.jl 92.22% <ø> (-0.05%) ⬇️
src/FixedPointNumbers.jl 88.34% <100.00%> (+0.59%) ⬆️

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 7f6575e...04e18b8. Read the comment docs.

@kimikage kimikage marked this pull request as ready for review August 29, 2020 05:26
@kimikage
Copy link
Collaborator Author

kimikage commented Aug 29, 2020

Benchmark

x_q0f7  = rand(Q0f7,  1000, 1000); y_q0f7  = rand(Q0f7,  1000, 1000);
x_q0f15 = rand(Q0f15, 1000, 1000); y_q0f15 = rand(Q0f15, 1000, 1000);
x_q3f4  = rand(Q3f4,  1000, 1000); y_q3f4  = rand(Q3f4,  1000, 1000);
x_q3f12 = rand(Q3f12, 1000, 1000); y_q3f12 = rand(Q3f12, 1000, 1000);

x_n0f8  = rand(N0f8,  1000, 1000); y_n0f8  = rand(N0f8,  1000, 1000);
x_n0f16 = rand(N0f16, 1000, 1000); y_n0f16 = rand(N0f16, 1000, 1000);
x_n4f4  = rand(N4f4,  1000, 1000); y_n4f4  = rand(N4f4,  1000, 1000);
x_n4f12 = rand(N4f12, 1000, 1000); y_n4f12 = rand(N4f12, 1000, 1000);

non_zero(x) = x === zero(x) ? eps(x) : x;
z(x::X) where X = clamp(x * 0.5 * rand(X), X);

w_q0f7  = non_zero.(y_q0f7);  z_q0f7  = z.(w_q0f7);
w_q0f15 = non_zero.(y_q0f15); z_q0f15 = z.(w_q0f15);
w_q3f4  = non_zero.(y_q3f4);  z_q3f4  = z.(w_q3f4);
w_q3f12 = non_zero.(y_q3f12); z_q3f12 = z.(w_q3f12);

w_n0f8  = non_zero.(y_n0f8);  z_n0f8  = z.(w_n0f8);
w_n0f16 = non_zero.(y_n0f16); z_n0f16 = z.(w_n0f16);
w_n4f4  = non_zero.(y_n4f4);  z_n4f4  = z.(w_n4f4);
w_n4f12 = non_zero.(y_n4f12); z_n4f12 = z.(w_n4f12);

@btime $z_q0f7  ./ $w_q0f7;
@btime $z_q0f15 ./ $w_q0f15;
@btime $z_q3f4  ./ $w_q3f4;
@btime $z_q3f12 ./ $w_q3f12;

@btime $z_n0f8  ./ $w_n0f8;
@btime $z_n0f16 ./ $w_n0f16;
@btime $z_n4f4  ./ $w_n4f4;
@btime $z_n4f12 ./ $w_n4f12;
@btime wrapping_fdiv.($x_q0f7 , $y_q0f7 );
@btime wrapping_fdiv.($x_q0f15, $y_q0f15);
@btime wrapping_fdiv.($x_q3f4 , $y_q3f4 );
@btime wrapping_fdiv.($x_q3f12, $y_q3f12);

@btime wrapping_fdiv.($x_n0f8 , $y_n0f8 );
@btime wrapping_fdiv.($x_n0f16, $y_n0f16);
@btime wrapping_fdiv.($x_n4f4 , $y_n4f4 );
@btime wrapping_fdiv.($x_n4f12, $y_n4f12);

@btime saturating_fdiv.($x_q0f7 , $y_q0f7 );
@btime saturating_fdiv.($x_q0f15, $y_q0f15);
@btime saturating_fdiv.($x_q3f4 , $y_q3f4 );
@btime saturating_fdiv.($x_q3f12, $y_q3f12);

@btime saturating_fdiv.($x_n0f8 , $y_n0f8 );
@btime saturating_fdiv.($x_n0f16, $y_n0f16);
@btime saturating_fdiv.($x_n4f4 , $y_n4f4 );
@btime saturating_fdiv.($x_n4f12, $y_n4f12);
julia> versioninfo()
Julia Version 1.5.1
Commit 697e782ab8 (2020-08-25 20:08 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-9.0.1 (ORCJIT, skylake)
Fixed before after Normed before after
z_q0f7 ./ w_q0f7 2.016 ms 1.880 ms z_n0f8 ./ w_n0f8 2.157 ms 1.656 ms
z_q0f15 ./ w_q0f15 2.435 ms 2.576 ms z_n0f16 ./ w_n0f16 3.017 ms 2.393 ms
z_q3f4 ./ w_q3f4 2.016 ms 1.940 ms z_n4f4 ./ w_n4f4 2.216 ms 1.691 ms
z_q3f12 ./ w_q3f12 2.458 ms 2.599 ms z_n4f12 ./ w_n4f12 3.611 ms 2.362 ms
Fixed Normed
wrapping_fdiv. (x_q0f7 , y_q0f7 ) 290.3 μs (x_n0f8 , y_n0f8 ) 286.8 μs
wrapping_fdiv. (x_q0f15, y_q0f15) 920.4 μs (x_n0f16, y_n0f16) 896.5 μs
wrapping_fdiv. (x_q3f4 , y_q3f4 ) 287.0 μs (x_n4f4 , y_n4f4 ) 286.7 μs
wrapping_fdiv. (x_q3f12, y_q3f12) 896.6 μs (x_n4f12, y_n4f12) 881.2 μs
saturating_fdiv. (x_q0f7 , y_q0f7 ) 272.3 μs (x_n0f8 , y_n0f8 ) 217.4 μs
saturating_fdiv. (x_q0f15, y_q0f15) 892.7 μs (x_n0f16, y_n0f16) 826.2 μs
saturating_fdiv. (x_q3f4 , y_q3f4 ) 270.5 μs (x_n4f4 , y_n4f4 ) 218.0 μs
saturating_fdiv. (x_q3f12, y_q3f12) 894.2 μs (x_n4f12, y_n4f12) 834.9 μs

@kimikage kimikage force-pushed the checked_fdiv branch 2 times, most recently from b8f066a to a82ceb4 Compare August 30, 2020 00:34
@kimikage
Copy link
Collaborator Author

JuliaLang/julia #37085 was merged. ("Normed{UInt128,100}"-->"Normed{UInt128, 100}")
So, shall we update the test first? (cf. #139 (comment))

Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting that we've had checked here a long time and I no longer remembered that...I can't remember the last time I divided FixedPoints, but I suspect it has come up.

@kimikage
Copy link
Collaborator Author

kimikage commented Aug 31, 2020

FYI, the Normed multiplication has also been checked so far. (cf. #213)

julia> 10N4f12 * 10N4f12 # v0.8.4
ERROR: ArgumentError: Normed{UInt16,12} is a 16-bit type representing 65536 values from 0.0 to 16.0037; cannot represent 100.0

julia> 10N4f12 * 10N4f12 # PR #213 (checked arithmetic for `Normed`)
ERROR: OverflowError: 10.0N4f12 * 10.0N4f12 overflowed for type N4f12

julia> 10Q4f11 * 10Q4f11 # PR #220 (wrapping arithmetic for `Fixed`)
4.0Q4f11

I will soon be asking for feedback on the default arithmetic in v0.9 and the state of arithmetic in v0.10 and later in the Discourse.

@timholy
Copy link
Member

timholy commented Aug 31, 2020

Yeah, I think with multiplication I always convert to float first.

This also changes the implementation of `/` for `Fixed`.
`/` (`checked_fdiv`) throws `DivideError` for division by zero and `OverflowError` for overflow.
@kimikage
Copy link
Collaborator Author

I ran rebase and moved the 100% tests to "test/common.jl".

@kimikage kimikage merged commit f7d7bbc into JuliaMath:master Aug 31, 2020
@kimikage kimikage deleted the checked_fdiv branch August 31, 2020 22:06
@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 kimikage mentioned 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.

2 participants