Support alpha(::Number) and ensure alpha/gray return compatible types #177

timholy · 2020-03-18T11:46:38Z

Closes #162. I decided to continue to support any ::Number as long as it can be converted to a supported type. Consequently

julia> using FixedPointNumbers, ColorTypes

julia> FixedPointNumbers.floattype(::Type{Complex{T}}) where T = Complex{floattype(T)}

julia> gray(1+0im)
1.0N0f8

julia> gray(1+1im)
ERROR: InexactError: Int64(1 + 1im)
Stacktrace:
 [1] Real at ./complex.jl:37 [inlined]
 [2] convert at ./number.jl:7 [inlined]
 [3] FixedPoint at /home/tim/.julia/packages/FixedPointNumbers/w2pxG/src/FixedPointNumbers.jl:56 [inlined]
 [4] convert at ./number.jl:7 [inlined]
 [5] gray(::Complex{Int64}) at /home/tim/.julia/dev/ColorTypes/src/traits.jl:37
 [6] top-level scope at REPL[5]:1

(Not sure if that is worth adding to the tests.) This is consistent with the general philosophy that types are about dispatch and algorithmic efficiency, but that functionality should depend primarily on value.

Breaking change: yes (conversions that can't be performed will now error, and some return values will be of different types).

codecov · 2020-03-18T12:08:24Z

Codecov Report

Merging #177 (93c6571) into master (306ed7d) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #177      +/-   ##
==========================================
+ Coverage   82.73%   82.75%   +0.02%     
==========================================
  Files           8        8              
  Lines         753      754       +1     
==========================================
+ Hits          623      624       +1     
  Misses        130      130

Impacted Files	Coverage Δ
src/traits.jl	`98.88% <100.00%> (+<0.01%)`	⬆️
src/types.jl	`91.38% <100.00%> (ø)`

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 306ed7d...93c6571. Read the comment docs.

kimikage · 2020-03-18T12:21:11Z

This is off-topic and optional, but see also #162 (comment)

timholy · 2020-03-18T12:44:25Z

I added that. There's no performance improvement with these new implementations, but I agree it's a better approach.

I also noticed an issue this caused with respect to alpha-constructors, but which was not tested in this package. I caught it by working on ColorVectorSpace; it makes sense to leave this open until that work comes to a conclusion.

kimikage · 2020-04-22T07:17:40Z

Oops. This seems to change the behavior of constructor with GrayLike arguments in "rare" cases.

ColorTypes.jl/src/types.jl

Lines 600 to 607 in 4bb8aa9

    
           const GrayLike = Union{Real,AbstractGray} 
        
           for C in (RGB, BGR, XRGB, RGBX, RGB24) 
        
               @eval (::Type{$C})(r::GrayLike, g::GrayLike, b::GrayLike) = $C(gray(r), gray(g), gray(b)) 
        
           end 
        
           for C in (RGB, BGR, XRGB, RGBX) 
        
               @eval (::Type{$C{T}})(r::GrayLike, g::GrayLike, b::GrayLike) where T = $C{T}(gray(r), gray(g), gray(b)) 
        
           end

I think real() is more appropriate there than gray() semantically.

Changing the ARGB32 constructor (cf. #183 (comment)) needs to add the ARGB32 constructor with GrayLike arguments for the backward compatibility (see below).

Therefore, we need to plan on which PR to make the changes to real(), and the merging order.

WRT GrayLike arguments, only opaque RGB constructors support them.

julia> ARGB32(1, 1, 1, Gray(0.5)) # a special case due to the side-effect of inconsistent implementation
ARGB32(1.0N0f8,1.0N0f8,1.0N0f8,0.502N0f8)

julia> ARGB(1, 1, 1, Gray(0.5))
ERROR: MethodError: no method matching ARGB(::Int64, ::Int64, ::Int64, ::Gray{Float64})

julia> AGray{Float32}(1, Gray(0.5))
ERROR: MethodError: no method matching AGray{Float32}(::Int64, ::Gray{Float64})

I don't like to confuse Gray with Real, so I'm not going to enhance the GrayLike support. However, it might be good to discuss that in a separate issue report.

I also noticed an issue this caused with respect to alpha-constructors

Is the problem related to this?

kimikage · 2020-04-22T07:33:02Z

test/types.jl

+@testset "transparent gray constructors" begin
+    for x in (1, N0f8(0.5), 0.5f0, 0.5)
+        T = isa(x, Int) ? N0f8 : typeof(x)
+        @test @inferred(AGray(x)) === @inferred(AGray(x, 1)) === AGray{T}(x, 1)
+        @test @inferred(GrayA(x)) === @inferred(GrayA(x, 1)) === GrayA{T}(x, 1)
+        g = Gray(x)
+        @test @inferred(AGray(g)) === @inferred(AGray(g, 1)) === AGray{T}(x, 1)
+        @test @inferred(GrayA(g)) === @inferred(GrayA(g, 1)) === GrayA{T}(x, 1)
+    end
+    @test AGray32(Gray(0.5)) === AGray32(Gray(0.5), 1) === AGray32(N0f8(0.5)) === AGray32(N0f8(0.5), 1) === reinterpret(AGray32, 0xff808080)
+end


What is the purpose of adding these tests? The name of the testset above is misleading?

Without the change to src/types.jl above, x = N0f8(0.5); AGray(x, 1) returns an AGray{Float32} because promote(x, 1) chooses Float32. But with an integer argument we know the only valid settings of alpha are 0 and 1, so a valid implementation would be to convert it a Bool first (drawing the error if that fails) and then call promote. But note that promote(x, true) chooses N0f8, so the final type produced is strongly dependent on internal details of the implementation of the constructor.

To me it seems that a more agnostic approach is to first convert alpha to a supported eltype and then do the promotion. Note that changing the signature of those same two methods to alpha::GrayLike would circumvent the error you noted in #177 (comment). Though I don't feel strongly that we need to support Gray values for the alpha channel, making it consistently throw an error would also be OK.

I finally got the purpose. 👍 (For such a purpose, I prefer to write the types extensionally, though.)

But with an integer argument we know the only valid settings of alpha are 0 and 1

I agree for the most part. However, so far, we have not checked the range of valid values for each "color" component. Actually , we only check the valid range of Normed "type", so AGray{N1f7}(1, 2) does not throw errors.
Therefore, I prefer the more agnostic approach.

I don't feel strongly that we need to support Gray values for the alpha channel,

I agree. However, it is not only problem with the alpha channel.

julia> RGB{N0f8}(Gray(1), 1, 1) RGB{N0f8}(1.0,1.0,1.0) julia> ARGB{N0f8}(Gray(1), 1, 1, 0.5) ERROR: MethodError: no method matching ARGB{Normed{UInt8,8}}(::Gray{Normed{UInt8,8}}, ::Int64, ::Int64, ::Float64)

"Unfortunately", the possible way to make them consistent may be to enhance the support for GrayLike, and I support the status quo. 😅

I do think it's reasonable to construct an RGB from gray channels, as it is the inverse operation of color separation.

timholy · 2020-06-18T10:03:57Z

This is now being proposed for merging.

kimikage · 2020-06-23T14:25:50Z

I don't think there is any problem with this PR, but I am thinking of a radical overhaul of the constructors (cf. #197).

I would like to merge PRs with less impacts first.

timholy · 2020-06-23T14:50:03Z

I would like to merge PRs with less impacts first.

Can you clarify your intent? (What are "less impacts"?) Meaning, should this PR wait on #197 (but #197 requires this, right?) or other open PRs or are you ust agreeing that this should merge before #197?

kimikage · 2020-06-23T15:02:00Z

I meant #183, #187 and (something like) #189. The PR #197 is speculative.

timholy · 2020-06-23T15:04:51Z

Sorry I didn't merge #183. Done now.

kimikage · 2020-06-23T16:32:48Z

WRT the problem with the eltype promotion (#177 (comment)), I think the following rule (which does not explicitly depend on N0f8) is better.

# the modified version of `promote_eltype()`
@inline function promote_args_type(::Type{C}, alpha::Ta, vals...) where {Ta, C <: TransparentColor}
    T = promote_type(map(typeof, vals)...)
    if T <: Union{Integer, FixedPoint} && Ta <: Integer
        _promote_args_type(eltype_ub(C), eltype_default(C), T)
    else
        _promote_args_type(eltype_ub(C), eltype_default(C), promote_type(T, Ta))
    end
end

However, it is difficult to apply the rule consistently in the current constructor methods.Therefore, I think gray(alpha) is a reasonable solution in this PR.

kimikage · 2020-06-25T07:37:15Z

This is my oversight, but some tests should be added into "conversions from/to real numbers", "transparent rgb constructors" and so on, as mentioned in #162 (comment),

Although the problem is rooted in the lack of consistency in the constructor design, but we need some regression tests to improve the constructors. Of course, we can also add the tests in a separate PR.

timholy · 2020-06-25T08:11:21Z

Sure, wherever you want to add the tests is fine with me. Since this PR is late in the scheduled-merge queue, don't worry about conflicts, we can rebase when we get back to this.

kimikage · 2020-07-10T08:56:23Z

FixedPointNumbers v0.8.2, which includes JuliaMath/FixedPointNumbers.jl#177, was registered.

kimikage · 2021-03-24T14:24:30Z

FYI, I rewrote the tests for this in #240, so there is a conflict. I think it would be easier to just cherry-pick the changes in src and rewrite the tests.

Breaking change: yes (conversions that can't be performed will now error)

kimikage · 2021-05-14T17:15:17Z

I ran rebase and updated the tests.

kimikage · 2021-05-14T17:18:15Z

Although the relaxation to Gray{T<:Real} requires reconsideration of the behavior of gray, I think this behavior is fine for v0.12.

kimikage · 2021-05-18T09:03:15Z

Note that the behavior of the constructors will be changed again in PR #197. It is not recommended to merge the fixes for this PR's changes into the main branches of the downstream packages.

…le types (#177)" This reverts commit 81297ff.

PR JuliaGraphics#243 and JuliaGraphics#177 had already fixed the `ones` throwing an exception. This updates the test.

PR #243 and #177 had already fixed the `ones` throwing an exception. This updates the test.

timholy mentioned this pull request Mar 18, 2020

Support floattype(Rational) and require <:AbstractFloat for fallback JuliaMath/FixedPointNumbers.jl#177

Merged

timholy added the breaking label Mar 18, 2020

This comment has been minimized.

Sign in to view

kimikage reviewed Apr 22, 2020

View reviewed changes

timholy force-pushed the teh/alphareal branch from 83d7e02 to 1074af6 Compare June 18, 2020 09:49

This was referenced Jun 18, 2020

Do not merge: Implement < as well as isless, zero & oneunit for more types #186

Closed

Redesign color arithmetic JuliaGraphics/ColorVectorSpace.jl#131

Merged

kimikage mentioned this pull request Jun 23, 2020

[RFC] Redesign color constructors #197

Merged

kimikage mentioned this pull request Jun 25, 2020

Is it necessary to add alpha(x::Number)? #162

Closed

kimikage mentioned this pull request Jun 27, 2020

Add tests for 1-arg rgb constructors #200

Merged

kimikage mentioned this pull request Jul 9, 2020

let mapc support Number types #207

Open

kimikage mentioned this pull request Feb 25, 2021

Version 0.10.10 #227

Merged

This was referenced Mar 12, 2021

[RFC] Decision on the definition of one #235

Closed

Add tests for constructors of TransparentGray and custom types #240

Merged

This was referenced Apr 3, 2021

[RFC] (Re-)define one for Colorant #243

Merged

Version 0.11.0 #231

Merged

kimikage mentioned this pull request Apr 8, 2021

ColorType.gray(x::Real) is no longer needed JuliaGraphics/ColorVectorSpace.jl#124

Closed

kimikage added this to the 0.12 milestone Apr 22, 2021

kimikage mentioned this pull request May 6, 2021

Version 0.12.0 #251

Closed

19 tasks

timholy added 3 commits May 15, 2021 02:09

Support alpha(::Number) and ensure alpha/gray return compatible types

b74cf07

Breaking change: yes (conversions that can't be performed will now error)

Fix the types produced by transparent constructors

3d877ed

Use reinterpret instead of dummy-arg for building N0f8

93c6571

kimikage force-pushed the teh/alphareal branch from 1074af6 to 93c6571 Compare May 14, 2021 17:11

kimikage merged commit 81297ff into master May 15, 2021

kimikage deleted the teh/alphareal branch May 15, 2021 07:51

johnnychen94 added a commit that referenced this pull request May 15, 2022

Revert "Support alpha(::Number) and ensure alpha/gray return compatib…

ea8b889

…le types (#177)" This reverts commit 81297ff.

kimikage mentioned this pull request Apr 23, 2024

Re-organization for v0.13 release #299

Open

kimikage added a commit to kimikage/ColorTypes.jl that referenced this pull request May 6, 2024

Remove broken test_broken for ones

aa6b50b

PR JuliaGraphics#243 and JuliaGraphics#177 had already fixed the `ones` throwing an exception. This updates the test.

kimikage mentioned this pull request May 6, 2024

Remove broken test_broken for ones #308

Merged

kimikage added a commit that referenced this pull request May 6, 2024

Remove broken test_broken for ones (#308)

e195457

PR #243 and #177 had already fixed the `ones` throwing an exception. This updates the test.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support alpha(::Number) and ensure alpha/gray return compatible types #177

Support alpha(::Number) and ensure alpha/gray return compatible types #177

timholy commented Mar 18, 2020 •

edited

Loading

codecov bot commented Mar 18, 2020 •

edited

Loading

kimikage commented Mar 18, 2020

timholy commented Mar 18, 2020

This comment has been minimized.

kimikage commented Apr 22, 2020 •

edited

Loading

kimikage Apr 22, 2020 •

edited

Loading

timholy Apr 22, 2020 •

edited

Loading

kimikage Apr 22, 2020

kimikage Apr 22, 2020

timholy Apr 22, 2020

timholy commented Jun 18, 2020

kimikage commented Jun 23, 2020

timholy commented Jun 23, 2020 •

edited

Loading

kimikage commented Jun 23, 2020

timholy commented Jun 23, 2020

kimikage commented Jun 23, 2020

kimikage commented Jun 25, 2020

timholy commented Jun 25, 2020

kimikage commented Jul 10, 2020

kimikage commented Mar 24, 2021

kimikage commented May 14, 2021

kimikage commented May 14, 2021

kimikage commented May 18, 2021

Support alpha(::Number) and ensure alpha/gray return compatible types #177

Support alpha(::Number) and ensure alpha/gray return compatible types #177

Conversation

timholy commented Mar 18, 2020 • edited Loading

codecov bot commented Mar 18, 2020 • edited Loading

Codecov Report

kimikage commented Mar 18, 2020

timholy commented Mar 18, 2020

This comment has been minimized.

kimikage commented Apr 22, 2020 • edited Loading

kimikage Apr 22, 2020 • edited Loading

Choose a reason for hiding this comment

timholy Apr 22, 2020 • edited Loading

Choose a reason for hiding this comment

kimikage Apr 22, 2020

Choose a reason for hiding this comment

kimikage Apr 22, 2020

Choose a reason for hiding this comment

timholy Apr 22, 2020

Choose a reason for hiding this comment

timholy commented Jun 18, 2020

kimikage commented Jun 23, 2020

timholy commented Jun 23, 2020 • edited Loading

kimikage commented Jun 23, 2020

timholy commented Jun 23, 2020

kimikage commented Jun 23, 2020

kimikage commented Jun 25, 2020

timholy commented Jun 25, 2020

kimikage commented Jul 10, 2020

kimikage commented Mar 24, 2021

kimikage commented May 14, 2021

kimikage commented May 14, 2021

kimikage commented May 18, 2021

timholy commented Mar 18, 2020 •

edited

Loading

codecov bot commented Mar 18, 2020 •

edited

Loading

kimikage commented Apr 22, 2020 •

edited

Loading

kimikage Apr 22, 2020 •

edited

Loading

timholy Apr 22, 2020 •

edited

Loading

timholy commented Jun 23, 2020 •

edited

Loading