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

Return type of * for x::Gray{Bool} and real number #148

Open
kimikage opened this issue Apr 3, 2021 · 7 comments
Open

Return type of * for x::Gray{Bool} and real number #148

kimikage opened this issue Apr 3, 2021 · 7 comments
Milestone

Comments

@kimikage
Copy link
Collaborator

kimikage commented Apr 3, 2021

I haven't made up my mind about what the return type should be if the definition of one for Colorant types is the real number 1. (cf. JuliaGraphics/ColorTypes.jl#235)
I found an odd type promotion in the case study.

julia> Gray{N0f8}(1) * 1
Gray{Float32}(1.0f0)

julia> Gray{N0f8}(1) * true
Gray{N0f8}(1.0)

julia> Gray{N0f8}(1) * 1N0f8
Gray{N0f8}(1.0)
julia> Gray{Bool}(1) * 1 # !?
Gray{BigFloat}(1.0)

julia> Gray{Bool}(1) * true # !?
Gray{Float32}(1.0f0)

julia> Gray{Bool}(1) * 1N0f8
Gray{N0f8}(1.0)
@kimikage
Copy link
Collaborator Author

kimikage commented Apr 8, 2021

As for multiplication (multype), it is easy to fix, but for other operations, it may be a Pandora's box.:sweat_smile:

@kimikage
Copy link
Collaborator Author

kimikage commented Apr 9, 2021

The root of the problem is that Bool has two aspects: the 1-bit "integer" representation and the quantized representation of the "real" number in [0, 1]. As for Gray{Bool}, the latter property is strong. However, the same is true for N0f8, but those types easily overflow, and Inf/NaN cannot be represented.
In such a context, the following is reasonable:

julia> 0x1 + Gray{N0f8}(0)
Gray{Float32}(0.0f0)

And the following is completely irrational:

julia> true + Gray{Bool}(0)
Gray{N0f8}(1.0)

It is obvious that the result of addition and subtraction between integers is an integer, and there is no advantage to N0f8 (other than that it is often used in image processing). Of course,

julia> true + Gray{Bool}(1)
ERROR: ArgumentError: 2 is an integer in the range 0-255, but integer inputs are encoded with the N0f8
  type, an 8-bit type representing 256 discrete values between 0 and 1.
  Consider dividing your input values by 255, for example: Gray{N0f8}(2/255)
  Or use `reinterpret(N0f8, x)` if `x` is a `UInt8`.
  See the READMEs for FixedPointNumbers and ColorTypes for more information.

Personally, I think the following seems relatively reasonable. However, it goes against the idea of treating Gray and Real the same way (which, as I've said many times, I don't like 😅):

julia> true + Gray{Bool}(1) # the `true` is a `Integer`
Gray{Float32}(2.0)

julia> Gray{Bool}(1) + Gray{Bool}(1) # similar to `Gray{N0f8}(1) + Gray{N0f8}(1)`
Gray{Bool}(false)

At least, there is no reason to maintain the backward compatibility with respect to the Gray{Bool} operations. This is because the current situation is as follows 😂 :

julia> Gray{Bool}(1) + Gray{Bool}(1)
Gray{BigFloat}(2.0)

@kimikage kimikage added this to the 1.0 milestone Apr 9, 2021
@kimikage
Copy link
Collaborator Author

kimikage commented Apr 9, 2021

Of course, it is obvious that the Gray{Bool} arithmetic operations have never been used in practice, and I have no practical interest in enhancing support for Gray{Bool}.
However, the current implementation is a collection of ad-hoc rules, and there are some inconsistencies. For example:

julia> Gray{N0f8}(1) + 0
Gray{Float32}(1.0f0)

julia> Gray24(1) + 0
Gray24(1.0N0f8)
julia> Gray(1.0) / Gray{N0f8}(0.6)
Gray{Float64}(1.6666666666666667)

julia> Gray(1.0) / 0.6N0f8
ERROR: ArgumentError: N0f8 is an 8-bit type representing 256 values from 0.0 to 1.0; cannot represent 1.6666666

@kimikage
Copy link
Collaborator Author

Proposal

cf: #153 (comment)

+ 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 Gray24 Gray24 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{Bool} Gray{N0f8} Gray{Float32} Gray{Float32} Gray{Float64}
Gray{N0f8} Gray{N0f8} Gray{N0f8} Gray{Float32} Gray{Float32} Gray{Float64}
Gray24 Gray24🆙 Gray24🆙 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{Float32}🆙 Gray{Float32} Gray{Float32} Gray{Float64}
Gray{N0f8} Gray{Float32}🆙 Gray{N0f8} Gray{Float32} Gray{Float32} Gray{Float64}
Gray24 Gray{Float32}🆙 Gray24🆙 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{Float32}🆙 Gray{Float32}🆙 Gray{Float32} Gray{Float64}
Gray{N0f8} Gray{Float32}🆙 Gray{N0f8} Gray{N0f8} Gray{Float32} Gray{Float64}
Gray24 Gray{Float32}🆙 Gray{N0f8} Gray24 Gray{Float32} Gray{Float64}
Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float32} Gray{Float64}

🆙 : proposal for v0.10.0

@johnnychen94
Copy link
Member

johnnychen94 commented May 11, 2021

LGTM.

Are there any plans to support the other side of /? For example, I see Gray{Bool}/Bool = Gray{Float32} 🆙 (If I understand the order correctly), but don't see Bool/Gray{Bool} in the table.

Current Gray{Bool}/Bool = Gray{Float64} is consistent with Bool/Bool = Float64. I guess people would be surprised when trying to dig into the details. Maybe we can complicate the logic by checking if typeof(true/true) === Float64?

@kimikage
Copy link
Collaborator Author

but don't see Bool/Gray{Bool} in the table.

I added the ::Number/::Gray cases, and updated (fixed) the PR.

cf. #153 (comment), #153 (comment)

/ Gray{Bool} Gray{N0f8} Gray24 Gray{Float32}
Bool Gray{Float32} Gray{Float32}🆙 Gray{Float32}🆙 Gray{Float32}
N0f8 Gray{Float32}🆙 Gray{N0f8} Gray24 Gray{Float32}
Int64 Gray{Float32} Gray{Float32} Gray{Float32}🆙 Gray{Float32}
Float32 Gray{Float32} Gray{Float32} Gray{Float32}🆙 Gray{Float32}
Float64 Gray{Float64} Gray{Float64} Gray{Float64}🆙 Gray{Float64}

There are several other cases, but the current tests for ColorVectorSpace are not so systematic to begin with.

Current Gray{Bool}/Bool = Gray{Float64} is consistent with Bool/Bool = Float64

You're right, but I think it's also non-intuitive to suddenly promote to Gray{Float64} when manipulating binary grayscale images. I think it is natural for Bool to behave in the same way as UInt8 in the most cases.

@kimikage
Copy link
Collaborator Author

Incidentally, my motivation for considering the return types and intermediate representations is to bring consistency to the new definition of one.

However, the more essential issue is related to the relaxation from RGB{T<:Fractional} to RGB{T<:Real}. When not only RGB{Bool} but also RGB{Int} and others become "valid" types, it is important to manage the rules so that unwanted promotions do not occur.

It is debatable what kind of rules are most preferable, but it is definitely beneficial to generalize the codes as much as possible and consolidate the rules in one place.

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

No branches or pull requests

2 participants