Skip to content

[RFC] Improve consistency of arithmetic with Bool #153

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.

Sign up for GitHub

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 2 commits into from
May 15, 2021
Merged

[RFC] Improve consistency of arithmetic with Bool #153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
840435e
Improve consistency of arithmetic with `Bool`
kimikage Apr 8, 2021
19551e6
Fix backward compatibility of `::Bool / ::Normed`
kimikage May 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 34 additions & 8 deletions src/ColorVectorSpace.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,21 @@ divtype(::Type{A}, ::Type{B}) where {A,B<:FixedPoint} = coltype(typeof(zero(A)/t
powtype(::Type{A}, ::Type{B}) where {A,B} = coltype(typeof(zero(A)^zero(B)))
sumtype(a::Colorant, b::Colorant) = coltype(sumtype(eltype(a),eltype(b)))

multype(::Type{Bool}, ::Type{B}) where {B} = B
multype(::Type{A}, ::Type{Bool}) where {A} = A <: Integer ? multype(A, N0f8) : A
multype(::Type{Bool}, ::Type{Bool}) = Bool
sumtype(::Type{Bool}, ::Type{B}) where {B} = B <: Integer ? N0f8 : sumtype(N0f8, B) # FIXME
sumtype(::Type{A}, ::Type{Bool}) where {A} = sumtype(Bool, A)
sumtype(::Type{Bool}, ::Type{Bool}) = N0f8 # FIXME
divtype(::Type{Bool}, ::Type{B}) where {B} = divtype(B <: Integer ? N0f8 : UInt8, B)
Copy link
Member

@johnnychen94 johnnychen94 May 11, 2021

Choose a reason for hiding this comment

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

Will this be more friendly to the compiler?

Suggested change
divtype(::Type{Bool}, ::Type{B}) where {B} = divtype(B <: Integer ? N0f8 : UInt8, B)
divtype(::Type{Bool}, ::Type{B}) where {B} = divtype(ifelse(B <: Integer, N0f8, UInt8), B)

Copy link
Collaborator Author

@kimikage kimikage May 11, 2021

Choose a reason for hiding this comment

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

Do you have a specific example?
At least for me, the fewer parentheses, the friendlier it is. 😄

Copy link
Member

@johnnychen94 johnnychen94 May 11, 2021

Choose a reason for hiding this comment

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

No, but I think it's perfectly fine to evaluate both the true and false cases here as they're constant DataType.

From ?ifelse:
In some cases, using ifelse instead of an if statement can eliminate the branch in generated code and provide
higher performance in tight loops.

The @code_native looks the same for this case so I don't know if it worths. Maybe Julia is just smart enough to optimize it...

Copy link
Collaborator Author

@kimikage kimikage May 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Even using ifelse does not completely suppress the branches, so I simply prefer to keep it short and readable.
In the first place, since this is a matter of constant propagation (or dead code elimination), I don't think it has anything to do with the branch generation.

divtype(::Type{Bool}, ::Type{B}) where {B<:FixedPoint} = divtype(N0f8, B) # FIXME
divtype(::Type{A}, ::Type{Bool}) where {A} = divtype(A, A <: FixedPoint ? N0f8 : UInt8) # FIXME
divtype(::Type{A}, ::Type{Bool}) where {A<:Integer} = Float32 # FIXME
divtype(::Type{Bool}, ::Type{Bool}) = divtype(N0f8, UInt8)
powtype(::Type{Bool}, ::Type{B}) where {B} = B <: Integer ? Bool : B
powtype(::Type{A}, ::Type{Bool}) where {A} = A
powtype(::Type{Bool}, ::Type{Bool}) = Bool

coltype(::Type{T}) where {T<:Fractional} = T
coltype(::Type{T}) where {T<:Number} = floattype(T)

Expand Down Expand Up @@ -168,6 +183,9 @@ function _div(x::AbstractFloat, y::Normed)
end
_div(x::AbstractFloat, y::Integer) = _mul(x, oneunit(x) / y)
_div(x::T, y::T) where {T} = x / y
_div(x::Bool, y::Bool) = (T = divtype(typeof(x), typeof(y)); _div(T(x), T(y)))
_div(x::FixedPoint, y::Bool) = (T = divtype(typeof(x), typeof(y)); T(_div(T(x), T(y)))) # FIXME
_div(x::Bool, y::FixedPoint) = (T = divtype(typeof(x), typeof(y)); T(_div(T(x), T(y)))) # FIXME
_div(x, y) = (T = divtype(typeof(x), typeof(y)); _div(T(x), T(y)))

@inline _mapc(::Type{C}, f, c) where {C<:MathTypes} = C(f.(channels(c))...)
Expand Down Expand Up @@ -241,10 +259,10 @@ function (/)(c::C, f::AbstractFloat) where {C<:Union{AbstractRGB, TransparentRGB
r = oneunit(divtype(eltype(c), typeof(f))) / f
_mapc(rettype(/, c, f), v -> v * r, c)
end
(+)(a::AbstractRGB, b::AbstractRGB) = rettype(+, a, b)(red(a)+red(b), green(a)+green(b), blue(a)+blue(b))
(-)(a::AbstractRGB, b::AbstractRGB) = rettype(-, a, b)(red(a)-red(b), green(a)-green(b), blue(a)-blue(b))
(+)(a::TransparentRGB, b::TransparentRGB) = rettype(+, a, b)(red(a)+red(b), green(a)+green(b), blue(a)+blue(b), alpha(a)+alpha(b))
(-)(a::TransparentRGB, b::TransparentRGB) = rettype(-, a, b)(red(a)-red(b), green(a)-green(b), blue(a)-blue(b), alpha(a)-alpha(b))
(+)(a::AbstractRGB, b::AbstractRGB) = _mapc(rettype(+, a, b), +, a, b)
(-)(a::AbstractRGB, b::AbstractRGB) = _mapc(rettype(-, a, b), -, a, b)
(+)(a::TransparentRGB, b::TransparentRGB) = _mapc(rettype(+, a, b), +, a, b)
(-)(a::TransparentRGB, b::TransparentRGB) = _mapc(rettype(-, a, b), -, a, b)

# New multiplication operators
function (⋅)(x::AbstractRGB, y::AbstractRGB)
Expand Down Expand Up @@ -290,18 +308,26 @@ middle(c::AbstractGray) = arith_colorant_type(c)(middle(gray(c)))
middle(x::C, y::C) where {C<:AbstractGray} = arith_colorant_type(C)(middle(gray(x), gray(y)))

(/)(n::Number, c::AbstractGray) = base_color_type(c)(_div(real(n), gray(c)))
(+)(a::AbstractGray, b::AbstractGray) = rettype(+, a, b)(gray(a)+gray(b))
(+)(a::TransparentGray, b::TransparentGray) = rettype(+, a, b)(gray(a)+gray(b), alpha(a)+alpha(b))
(-)(a::AbstractGray, b::AbstractGray) = rettype(-, a, b)(gray(a)-gray(b))
(-)(a::TransparentGray, b::TransparentGray) = rettype(-, a, b)(gray(a)-gray(b), alpha(a)-alpha(b))
(+)(a::AbstractGray, b::AbstractGray) = _mapc(rettype(+, a, b), +, a, b)
(+)(a::AbstractGray{Bool}, b::AbstractGray{Bool}) = _mapc(rettype(+, a, b), ⊻, a, b)
(+)(a::TransparentGray, b::TransparentGray) = _mapc(rettype(+, a, b), +, a, b)
(-)(a::AbstractGray, b::AbstractGray) = _mapc(rettype(-, a, b), -, a, b)
(-)(a::AbstractGray{Bool}, b::AbstractGray{Bool}) = _mapc(rettype(-, a, b), ⊻, a, b)
(-)(a::TransparentGray, b::TransparentGray) = _mapc(rettype(-, a, b), -, a, b)
(*)(a::AbstractGray, b::AbstractGray) = a ⊙ b
(^)(a::AbstractGray, b::Integer) = rettype(^, a, b)(gray(a)^convert(Int,b))
(^)(a::AbstractGray, b::Real) = rettype(^, a, b)(gray(a)^b)
(/)(a::AbstractGray, b::AbstractGray) = rettype(/, a, b)(_div(gray(a), gray(b)))
(+)(a::AbstractGray, b::Number) = base_color_type(a)(gray(a)+b)
(+)(a::AbstractGray{Bool}, b::Number) = arith_colorant_type(a){sumtype(Bool, typeof(b))}(gray(a) + b)
(+)(a::AbstractGray{Bool}, b::Bool) = a + N0f8(b) # FIXME
(+)(a::Number, b::AbstractGray) = b+a
(-)(a::AbstractGray, b::Number) = base_color_type(a)(gray(a)-b)
(-)(a::Number, b::AbstractGray) = base_color_type(b)(a-gray(b))
(-)(a::AbstractGray{Bool}, b::Number) = arith_colorant_type(a){sumtype(Bool, typeof(b))}(gray(a) - b)
(-)(a::Number, b::AbstractGray{Bool}) = arith_colorant_type(b){sumtype(typeof(a), Bool)}(a - gray(b))
(-)(a::AbstractGray{Bool}, b::Bool) = a - N0f8(b) # FIXME
(-)(a::Bool, b::AbstractGray{Bool}) = N0f8(a) - b # FIXME

(⋅)(x::C, y::C) where {C<:AbstractGray} = _mul(gray(x), gray(y))
(⊗)(x::C, y::C) where {C<:AbstractGray} = x ⊙ y
Expand Down
96 changes: 93 additions & 3 deletions test/runtests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -607,13 +607,103 @@ ColorTypes.comp2(c::RGBA32) = alpha(c)
@test cf ⋅ cf === (Float64(red(cf))^2 + Float64(green(cf))^2 + Float64(blue(cf))^2)/3
end

@testset "arithmetic with Bool" begin
@testset "arithmetic with Bool" begin # issue 148
cb = Gray{Bool}(1)
@test @inferred(+cb) === cb
@test @inferred(-cb) === cb # wrapped around
@test_broken @inferred(one(cb) * cb) === cb
@test @inferred(one(cb) * cb) === cb
@test oneunit(cb) === Gray(true)
# TODO: add more tests (cf. issue #148)

@testset "vs. Bool" begin
@test_broken @inferred(cb + true) === @inferred(true + cb) === Gray{Float32}(2)
@test_broken @inferred(cb - true) === Gray{Float32}(0)
@test_broken @inferred(true - cb) === Gray{Float32}(0)
@test @inferred(cb + false) === @inferred(false + cb) === Gray{N0f8}(1) # v0.9 behavior
@test @inferred(cb - true) === Gray{N0f8}(0) # v0.9 behavior
@test @inferred(true - cb) === Gray{N0f8}(0) # v0.9 behavior
@test @inferred(cb * true) === @inferred(true * cb) === Gray{Bool}(1)
@test @inferred(cb / true) === Gray{Float32}(1)
@test @inferred(cb / false) === Gray{Float32}(Inf32)
@test @inferred(true / cb) === Gray{Float32}(1)
@test @inferred(cb^true) === cb
end
@testset "vs. Gray{Bool}" begin
@test @inferred(cb + Gray(true)) === @inferred(Gray(true) + cb) === Gray{Bool}(0) # wrapped around
@test @inferred(cb - Gray(true)) === Gray{Bool}(0)
@test @inferred(Gray(false) - cb) === Gray{Bool}(1) # wrapped around
@test @inferred(cb * Gray(true)) === @inferred(Gray(true) * cb) === Gray{Bool}(1)
@test @inferred(cb / Gray(true)) === Gray{Float32}(1)
@test @inferred(cb / Gray(false)) === Gray{Float32}(Inf32)
end
@testset "vs. Int" begin
@test_broken @inferred(cb + 2) === @inferred(2 + cb) === Gray{Float32}(3)
@test_broken @inferred(cb - 2) === Gray{Float32}(-1)
@test_broken @inferred(2 - cb) === Gray{Float32}(1)
@test @inferred(cb + 0) === @inferred(0 + cb) === Gray{N0f8}(1) # v0.9 behavior
@test @inferred(cb - 1) === Gray{N0f8}(0) # v0.9 behavior
@test @inferred(2 - cb) === Gray{N0f8}(1) # v0.9 behavior
@test @inferred(cb * 2) === @inferred(2 * cb) === Gray{Float32}(2)
@test @inferred(cb / 2) === Gray{Float32}(0.5)
@test @inferred(2 / cb) === Gray{Float32}(2)
@test @inferred(cb^1) === cb
end
# vs. Float32 and Gray{Float32}
@testset "vs. $(typeof(x))" for x in (0.5f0, Gray(0.5f0))
@test @inferred(cb + x) === @inferred(x + cb) === Gray{Float32}(1.5)
@test @inferred(cb - x) === Gray{Float32}(0.5)
@test @inferred(x - cb) === Gray{Float32}(-0.5)
@test @inferred(cb * x) === @inferred(x * cb) === Gray{Float32}(0.5)
@test @inferred(cb / x) === Gray{Float32}(2)
@test @inferred(x / cb) === Gray{Float32}(0.5)
if x isa Real
@test @inferred(cb^x) === Gray{Float32}(1)
else
@test @inferred(x^true) === Gray{Float32}(0.5)
end
end
# vs. N0f8 and Gray{N0f8}
@testset "vs. $(typeof(x))" for x in (0.6N0f8, Gray(0.6N0f8))
@test @inferred(cb + x) === @inferred(x + cb) === Gray{N0f8}(1N0f8 + 0.6N0f8)
@test @inferred(cb - x) === Gray{N0f8}(1N0f8 - 0.6N0f8)
@test @inferred(x - cb) === Gray{N0f8}(0.6N0f8 - 1N0f8)
@test @inferred(cb * x) === @inferred(x * cb) === Gray{N0f8}(0.6)
@test_broken @inferred(cb / x) === Gray{Float32}(1 / 0.6)
@test_broken @inferred(x / cb) === Gray{Float32}(0.6)
@test @inferred(cb / oneunit(x)) === Gray{N0f8}(1) # v0.9 behavior
@test @inferred(x / cb) === Gray{N0f8}(0.6) # v0.9 behavior
if x isa Gray
@test_broken @inferred(true / x) === Gray{Float32}(1 / 0.6)
@test @inferred(true / Gray(1)) === Gray{N0f8}(1.0) # v0.9 behavior
@test @inferred(x^true) === Gray{N0f8}(0.6)
end
end

@testset "vs. $(typeof(c)) multiplications" for c in (Gray(true), Gray(0.5f0), Gray(0.6N0f8))
@test @inferred(cb ⋅ c) === @inferred(c ⋅ cb) === gray(c)
@test @inferred(cb ⊙ c) === @inferred(c ⊙ cb) === c
@test @inferred(cb ⊗ c) === @inferred(c ⊗ cb) === c
end
cf = RGB{Float32}(0.1, 0.2, 0.3)
@test @inferred(cf + cb) === @inferred(cb + cf) === RGB{Float32}(1.1, 1.2, 1.3)
@test @inferred(cf - cb) === RGB{Float32}(-0.9, -0.8, -0.7)
@test @inferred(cb - cf) === RGB{Float32}(0.9, 0.8, 0.7)
cu = RGB{N0f8}(0.1, 0.2, 0.3)
@test @inferred(cu + cb) === @inferred(cb + cu) === mapc(v -> v + 1N0f8, cu) # wrapped around
@test @inferred(cu - cb) === mapc(v -> v - 1N0f8, cu) # wrapped around
@test @inferred(cb - cu) === mapc(v -> 1N0f8 - v, cu)
@testset "vs. $(typeof(c))" for c in (cf, cu)
@test @inferred(c * true) === @inferred(true * c) === c
if c === cu
@test_broken @inferred(c / true) === c / 1
@test @inferred(c / true) == c # v0.9 behavior
else
@test @inferred(c / true) === c / 1
end
@test @inferred(cb ⋅ c) === @inferred(c ⋅ cb) === Gray(1) ⋅ c
@test @inferred(cb ⊙ c) === @inferred(c ⊙ cb) === c
@test @inferred(cb ⊗ c) === Gray(1) ⊗ c
@test @inferred(c ⊗ cb) === c ⊗ Gray(1)
end
end

@testset "Complement" begin
Expand Down