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.

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

Conversation

kimikage
Copy link
Collaborator

@kimikage kimikage commented Apr 9, 2021

I'm going to create some separate PRs for the parts not related to Bool.
This PR only fixes return types which are clearly unreasonable. Other inconsistent return types are leaved for backward compatibility for now.

I feel the need to make the code more generalized before we can achieve v1.0. However, I am not sure when to do that. (Edit: see #156)
Mostly improved.

@codecov
Copy link

codecov bot commented Apr 9, 2021

Codecov Report

Merging #153 (19551e6) into master (32373c7) will increase coverage by 0.31%.
The diff coverage is 90.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #153      +/-   ##
==========================================
+ Coverage   84.97%   85.29%   +0.31%     
==========================================
  Files           2        2              
  Lines         213      238      +25     
==========================================
+ Hits          181      203      +22     
- Misses         32       35       +3     
Impacted Files Coverage Δ
src/ColorVectorSpace.jl 89.82% <90.90%> (-0.23%) ⬇️

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 32373c7...19551e6. Read the comment docs.

@kimikage
Copy link
Collaborator Author

kimikage commented May 9, 2021

v0.9.4

+ Bool N0f8 Int64 Float32 Float64
Gray{Bool} Gray{N0f8}🍇 Gray{N0f8} Gray{N0f8}🍇 Gray{Float32} Gray{Float64}
Gray{N0f8} Gray{N0f8} Gray{N0f8} Gray{Float32} Gray{Float32} Gray{Float64}
Gray24 Gray24 Gray24 Gray24🍇 Gray24🍇 Gray24🍇
Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float64}
+ Gray{Bool} Gray{N0f8} Gray24 Gray{Float32} Gray{Float64}
Gray{Bool} Gray{Bool} Gray{N0f8} Gray{N0f8} Gray{Float32} Gray{Float64}
Gray{N0f8} Gray{N0f8} Gray{N0f8} Gray{N0f8} Gray{Float32} Gray{Float64}
Gray24 Gray{N0f8} Gray{N0f8} Gray24 Gray{Float32} Gray{Float64}
Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float64}
* Bool N0f8 Int64 Float32 Float64
Gray{Bool} Gray{Float32} Gray{N0f8} Gray{BigFloat} Gray{Float32} Gray{Float64}
Gray{N0f8} Gray{N0f8} Gray{N0f8} Gray{Float32} Gray{Float32} Gray{Float64}
Gray24 Gray{N0f8}🍇 Gray{N0f8}🍇 Gray{Float32} Gray{Float32} Gray{Float64}
Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float64}
* Gray{Bool} Gray{N0f8} Gray24 Gray{Float32} Gray{Float64}
Gray{Bool} Gray{Float32} Gray{N0f8} Gray{N0f8} Gray{Float32} Gray{Float64}
Gray{N0f8} Gray{N0f8} Gray{N0f8} Gray{N0f8} Gray{Float32} Gray{Float64}
Gray24 Gray{N0f8} Gray{N0f8} Gray24 Gray{Float32} Gray{Float64}
Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float64}
/ Bool N0f8 Int64 Float32 Float64
Gray{Bool} Gray{Float64} Gray{N0f8}🍇 Gray{Float64} Gray{Float32} Gray{Float64}
Gray{N0f8} Gray{N0f8}🍇 Gray{N0f8} Gray{Float32} Gray{Float32} Gray{Float64}
Gray24 Gray{N0f8}🍇 Gray{N0f8}🍇 Gray{Float32} Gray{Float32} Gray{Float64}
Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float64}
/ Gray{Bool} Gray{N0f8} Gray24 Gray{Float32} Gray{Float64}
Gray{Bool} Gray{Float64} Gray{N0f8}🍇 Gray{N0f8}🍇 Gray{Float32} Gray{Float64}
Gray{N0f8} Gray{N0f8}🍇 Gray{N0f8} Gray{N0f8} Gray{Float32} Gray{Float64}
Gray24 Gray{N0f8}🍇 Gray{N0f8} Gray24 Gray{Float32} Gray{Float64}
Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float64}
(Added) / Gray{Bool} Gray{N0f8} Gray24 Gray{Float32}
Bool Gray{Float64} Gray{N0f8}🍇 Gray24🍇 Gray{Float32}
N0f8 Gray{N0f8}🍇 Gray{N0f8} Gray24 Gray{Float32}
Int64 Gray{Float64} Gray{Float32} Gray24🍇 Gray{Float32}
Float32 Gray{Float32} Gray{Float32} Gray24🍇 Gray{Float32}
Float64 Gray{Float64} Gray{Float64} Gray24🍇 Gray{Float64}
(Added) ^ Bool Int64 Float32 Float64
Gray{Bool} Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float64}
Gray{N0f8} Gray{N0f8} Gray{N0f8} Gray{Float32} Gray{Float64}
Gray24 Gray{N0f8}🍇 Gray{N0f8}🍇 Gray{Float32} Gray{Float64}
Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float64}

❌ : to be fixed as a bug
🍇 : to be reconsidered in v0.10.0

kimikage added 2 commits May 10, 2021 22:16
This only fixes return types which are clearly unreasonable.
Other inconsistent return types are leaved for backward compatibility for now.
@kimikage kimikage force-pushed the bool_arithmetic branch 2 times, most recently from 69a4b58 to 5931c44 Compare May 11, 2021 02:41
@kimikage
Copy link
Collaborator Author

kimikage commented May 11, 2021

this PR

+ Bool N0f8 Int64 Float32 Float64
Gray{Bool} Gray{N0f8}🍇 Gray{N0f8} Gray{N0f8}🍇 Gray{Float32} Gray{Float64}
Gray{N0f8} Gray{N0f8} Gray{N0f8} Gray{Float32} Gray{Float32} Gray{Float64}
Gray24 Gray24 Gray24 Gray24🍇 Gray24🍇 Gray24🍇
Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float64}
+ Gray{Bool} Gray{N0f8} Gray24 Gray{Float32} Gray{Float64}
Gray{Bool} Gray{Bool}🆙 Gray{N0f8} Gray{N0f8} Gray{Float32} Gray{Float64}
Gray{N0f8} Gray{N0f8} Gray{N0f8} Gray{N0f8} Gray{Float32} Gray{Float64}
Gray24 Gray{N0f8} Gray{N0f8} Gray24 Gray{Float32} Gray{Float64}
Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float64}
* Bool N0f8 Int64 Float32 Float64
Gray{Bool} Gray{Bool}🆙 Gray{N0f8} Gray{Float32}🆙 Gray{Float32} Gray{Float64}
Gray{N0f8} Gray{N0f8} Gray{N0f8} Gray{Float32} Gray{Float32} Gray{Float64}
Gray24 Gray{N0f8}🍇 Gray{N0f8}🍇 Gray{Float32} Gray{Float32} Gray{Float64}
Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float64}
* Gray{Bool} Gray{N0f8} Gray24 Gray{Float32} Gray{Float64}
Gray{Bool} Gray{Bool}🆙 Gray{N0f8} Gray{N0f8} Gray{Float32} Gray{Float64}
Gray{N0f8} Gray{N0f8} Gray{N0f8} Gray{N0f8} Gray{Float32} Gray{Float64}
Gray24 Gray{N0f8} Gray{N0f8} Gray24 Gray{Float32} Gray{Float64}
Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float64}
/ Bool N0f8 Int64 Float32 Float64
Gray{Bool} Gray{Float32}🆙 Gray{N0f8}🍇 Gray{Float32}🆙 Gray{Float32} Gray{Float64}
Gray{N0f8} Gray{N0f8}🍇 Gray{N0f8} Gray{Float32} Gray{Float32} Gray{Float64}
Gray24 Gray{N0f8}🍇 Gray{N0f8}🍇 Gray{Float32} Gray{Float32} Gray{Float64}
Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float64}
/ Gray{Bool} Gray{N0f8} Gray24 Gray{Float32} Gray{Float64}
Gray{Bool} Gray{Float32}🆙 Gray{N0f8}🍇 Gray{N0f8}🍇 Gray{Float32} Gray{Float64}
Gray{N0f8} Gray{N0f8}🍇 Gray{N0f8} Gray{N0f8} Gray{Float32} Gray{Float64}
Gray24 Gray{N0f8}🍇 Gray{N0f8} Gray24 Gray{Float32} Gray{Float64}
Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float64}
(Added) / Gray{Bool} Gray{N0f8} Gray24 Gray{Float32}
Bool Gray{Float32}🆙 Gray{N0f8}🍇 Gray24🍇 Gray{Float32}
N0f8 Gray{N0f8}🍇 Gray{N0f8} Gray24 Gray{Float32}
Int64 Gray{Float32}🆙 Gray{Float32} Gray24🍇 Gray{Float32}
Float32 Gray{Float32} Gray{Float32} Gray24🍇 Gray{Float32}
Float64 Gray{Float64} Gray{Float64} Gray24🍇 Gray{Float64}
(Added) ^ Bool Int64 Float32 Float64
Gray{Bool} Gray{Bool}🆙 Gray{Bool}🆙 Gray{Float32} Gray{Float64}
Gray{N0f8} Gray{N0f8} Gray{N0f8} Gray{Float32} Gray{Float64}
Gray24 Gray{N0f8}🍇 Gray{N0f8}🍇 Gray{Float32} Gray{Float64}
Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float64}

🆙 : fixed in this PR
🍇 : to be reconsidered in v0.10.0

@kimikage kimikage marked this pull request as ready for review May 11, 2021 02:50
@kimikage kimikage changed the title [RFC/WIP] Improve consistency of arithmetic with Bool [RFC] Improve consistency of arithmetic with Bool May 11, 2021
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

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

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

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

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.

@kimikage
Copy link
Collaborator Author

I will merge this and release v0.9.5.
If you are lucky enough to find problems, I will fix them in v0.9.6. 😄

@kimikage kimikage merged commit f177980 into JuliaGraphics:master May 15, 2021
@kimikage kimikage deleted the bool_arithmetic branch May 15, 2021 00:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants