Create a Complement type #189

mkitti · 2023-11-29T16:51:39Z

This adds a Complement{C,T,N} type.

The complement is to be interpreted as a lazy complement.

~~Math on Complement affects its underlying Colorant.~~

mkitti · 2023-11-29T16:52:11Z

I will probably move this into its file that is included near the top of the file.

codecov · 2023-11-29T16:53:17Z

Codecov Report

Attention: Patch coverage is 0% with 46 lines in your changes are missing coverage. Please review.

Project coverage is 77.63%. Comparing base (4d8eaaa) to head (7245c8b).

❗ Current head 7245c8b differs from pull request most recent head 070de6b. Consider uploading reports for the commit 070de6b to get more accurate results

Files	Patch %	Lines
src/ColorVectorSpace.jl	0.00%	46 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master     #189       +/-   ##
===========================================
- Coverage   91.04%   77.63%   -13.41%     
===========================================
  Files           3        3               
  Lines         268      313       +45     
===========================================
- Hits          244      243        -1     
- Misses         24       70       +46

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mkitti · 2023-11-29T23:52:00Z

@tlnagy @timholy could you review?

tlnagy · 2023-12-06T21:48:42Z

I don't have a ton of experience with ColorVectorSpace, but this seems like a reasonable implementation.

timholy

I like introducing Complement but I worry the interpretation here isn't consistent. Perhaps you can explain more about why you think it should work this way?

timholy · 2023-12-20T01:45:52Z

src/ColorVectorSpace.jl

+`convert` to `C` at which point `complement` will be applied. Like `complement`
+the alpha channel is not modified.
+
+The main interpretation of this type is to invert the *interpretation* of the


Two "interpretation"s on one line. Do you mean "main application"?

timholy · 2023-12-20T08:48:09Z

src/ColorVectorSpace.jl

+Mathematical operations on `Complement` will affect the underlying
+`Colorant` rather than changing the value of the complement. That is


I'm worried about this behavior---generally, we value the interpretation above the representation. What should Complement(0.1) + 0.1 return? I'd say 1.0 and not 0.2. Complement(0.0) is white, but with this logic, Complement(0.0) == 1.0 should return false.

Perhaps Complement(0.1) + 0.1 should be Complement(0.0) then. That is Complement(x) + y is Complement(x-y).

mkitti · 2023-12-20T14:30:20Z

I'm tempted to drop the math part from line 315 down

tlnagy · 2024-01-09T19:09:07Z

Bump, this would be nice to have merged for tlnagy/TiffImages.jl#134

src/ColorVectorSpace.jl

mkitti · 2024-01-09T19:30:51Z

@tlnagy what are the minimum features that you need?

tlnagy · 2024-01-09T20:12:49Z

I think it would be nice for Complemented values to be realized anytime you look at them. So it would be lazy until you accessed the values (like for display)

julia> res = Gray{N0f8}.(Matrix(I, 5, 5));

julia> Complement.(res) # doesn't work

TypeError: in typeassert, expected Complement{N0f8, 1, Gray{N0f8}}, got a value of type Gray{N0f8}

Stacktrace:
 [1] setindex!
   @ ./array.jl:971 [inlined]
 [2] setindex!
   @ ./multidimensional.jl:670 [inlined]
 [3] macro expansion
   @ ./broadcast.jl:974 [inlined]
 [4] macro expansion
   @ ./simdloop.jl:77 [inlined]
 [5] copyto!
   @ ./broadcast.jl:973 [inlined]
 [6] copyto!
   @ ./broadcast.jl:926 [inlined]
 [7] copy
   @ ./broadcast.jl:898 [inlined]
 [8] materialize(bc::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{2}, Nothing, Type{Complement}, Tuple{Matrix{Gray{N0f8}}}})
   @ Base.Broadcast ./broadcast.jl:873
 [9] top-level scope
   @ In[12]:1

julia> convert.(Gray{N0f8}, Complement.(res)) # does work, but clunky

For TiffImages the idea would be to return an <: AbstractArray{Complement{Gray{N0f8}}, 3} and the users wouldn't even be able to tell that the complementing is happening when they access the data. It should just happen behind the scenes and behave identically to the standard <: AbstractArray{Gray{N0f8}, 3} containers.

mkitti · 2024-01-09T21:42:13Z

Wait... why doesn't Complement.(res) work?

julia> using ColorVectorSpace, Colors, FixedPointNumbers, LinearAlgebra

julia> res = Gray{N0f8}.(Matrix(I, 5, 5));

julia> Complement.(res)
5×5 Array{Complement{N0f8},2} with eltype Complement{N0f8, 1, Gray{N0f8}}:
 Complement(Gray{N0f8}(1.0))  Complement(Gray{N0f8}(0.0))  …  Complement(Gray{N0f8}(0.0))  Complement(Gray{N0f8}(0.0))
 Complement(Gray{N0f8}(0.0))  Complement(Gray{N0f8}(1.0))     Complement(Gray{N0f8}(0.0))  Complement(Gray{N0f8}(0.0))
 Complement(Gray{N0f8}(0.0))  Complement(Gray{N0f8}(0.0))     Complement(Gray{N0f8}(0.0))  Complement(Gray{N0f8}(0.0))
 Complement(Gray{N0f8}(0.0))  Complement(Gray{N0f8}(0.0))     Complement(Gray{N0f8}(1.0))  Complement(Gray{N0f8}(0.0))
 Complement(Gray{N0f8}(0.0))  Complement(Gray{N0f8}(0.0))     Complement(Gray{N0f8}(0.0))  Complement(Gray{N0f8}(1.0))

I see that it should display, but it should "work".

tlnagy · 2024-01-09T22:15:25Z

I had ImageShow loaded in a Jupyter notebook, maybe that's why?

IMO the lazy complement should be realized on display.

mkitti · 2024-01-09T22:27:17Z

I think the problem is that Complement is a Colorant rather than a Color. Perhaps it should be a Color.

mkitti · 2024-01-10T02:54:20Z

I made Complement <: Color, so now it renders in VS Code. I'm not sure if that is the best design at the moment. Maybe I should write particular display for Complement?

tlnagy · 2024-01-10T04:52:03Z

Would it be possible to write a display for Complement that just punts to the correct display based on the wrapped type? i.e. Complement{T, N, C <: Colorant{T,N}} realizes the transform and then punts to the display for Colorant{T, N} etc

mkitti · 2024-01-10T05:48:44Z

I believe so.

My understanding is that this is implemented in Colors.jl:

https://github.com/JuliaGraphics/Colors.jl/blob/master/src%2Fdisplay.jl

It looks like we just need to overload show for Complement. I think the implementation could be simple in that we would do the conversion to C, the underlying Colorant, and then invoke its show method.

mkitti · 2024-01-10T10:11:54Z

I have an implementation of display now.

julia> using ColorVectorSpace, Colors, FixedPointNumbers, LinearAlgebra

julia> res = rand(RGB{Float64}, 10, 10)

julia> c_res = reinterpret(Complement, res) # less allocations

juila> using TestImages

julia> cameraman = testimage("cameraman.tif")

juila> c_cameraman = reinterpret(Complement, cameraman) # no allocation complement

julia> ca_cameraman = ComplementArray(c_cameraman) # no allocation, now of element type Gray{N0f8}

julia> [cameraman ca_cameraman]

mkitti · 2024-01-10T18:21:54Z

I'm getting tempted to implement this in its own package. It seems slightly out of place here and we might be able to iterate on it faster if it were independent.

tlnagy · 2024-01-10T19:03:22Z

Looks great! There's a slight issue though with display for larger images:

using ColorVectorSpace
using Colors
using FixedPointNumbers
using ImageShow
using LinearAlgebra

res = Gray{N0f8}.(Matrix(I, 500, 500)) # no warning
#snip

@time reinterpret(Complement, res) # works but with warning
#snip
┌ Warning: Output swatches are reduced due to the large size (500×500).
│ Load the ImageShow package for large images.
└ @ Colors ~/.julia/packages/Colors/mIuXl/src/display.jl:15

This happens despite ImageShow being loaded and this warning doesn't happen for non-Complemented images.

EDIT: wrapping with ComplementArray fixes this:

ComplementArray(reinterpret(Complement, res)) # no warning

mkitti · 2024-01-24T05:16:16Z

Should we push to get this merged now or should I break out into a package to iterate on this independently?

tlnagy · 2024-01-24T06:10:37Z

Given that complement lives here, I think it makes sense to have the lazy version here as well. But I'm willing to defer to other folks who know the Colors ecosystem better. Would be nice to have merged either way.

timholy · 2024-01-24T17:29:08Z

Sorry, I'll try to get this a review by next Monday. I got started and remembered getting worried about something fundamental, but that was long enough ago that I've forgotten.

mkitti · 2024-01-25T05:39:22Z

There's been considerable changes since you last took a look, Tim.

Complement subtypes Color instead of Colorant
I changed the order of parameters from {T,N,C} to {C,T,N}
I dropped arithmetic on Complement
Accessing the components of Complement returns the complement of the parent Colorant.

mkitti · 2024-01-25T05:44:08Z

Implementation of Base.reinterpret(::Type{Complement}, array::AbstractArray{C}) where {T, N, C <: Colorant{T,N}} may be an abuse because Complement by itself is not fully specified with parameters. However, it is inferrable that we mean Complement{C,T,N}

timholy

I'm on board with the general idea. Needs tests.

src/ColorVectorSpace.jl

timholy · 2024-01-27T22:23:58Z

src/ColorVectorSpace.jl

+Base.parent(a::ComplementArray) = a.parent
+Base.size(a::ComplementArray) = size(a.parent)
+function Base.getindex(a::ComplementArray{T}, I::Vararg{Int,N})::T where {T,N}
+    getindex(a.parent, I...)


I'm confused, shouldn't this be complement(getindex(a.parent, I...))? Basically this works like mappedarray(complement, A), right?

Which begs the question, do we actually need ComplementArray? But perhaps I misunderstand the intention.

Is this still a TODO? Given that I don't know what this is about, I'm unsure.

timholy · 2024-01-27T22:25:25Z

src/ColorVectorSpace.jl

+    getindex(a.parent, I...)
+end
+function Base.setindex!(a::ComplementArray{T}, v, I::Vararg{Int, N}) where {T,N}
+    setindex!(a.parent, convert(eltype(a.parent), v))


The convert will happen automatically, no need to do it manually.

timholy · 2024-01-27T22:26:07Z

src/ColorVectorSpace.jl

+function Base.setindex!(a::ComplementArray{T}, v, I::Vararg{Int, N}) where {T,N}
+    setindex!(a.parent, convert(eltype(a.parent), v))
+end
+Base.IndexStyle(a::ComplementArray) = IndexStyle(a.parent)


If you do this you should also implement getindex for a single Int.

Given your Vararg{Int, N} for any N I doubt that's really needed.

timholy · 2024-01-27T22:27:03Z

src/ColorVectorSpace.jl

+Base.IndexStyle(a::ComplementArray) = IndexStyle(a.parent)
+
+function Base.show(io::IO, mime::MIME"image/svg+xml", c::Complement{C}) where C
+    show(io, mime, C(c))


What happens without this method? I would think that if you have all the abstractions correct, this Just Works without needing a new method. Similar thoughts about the other methods below.

tlnagy · 2024-02-27T01:27:47Z

Bump @mkitti, this would be great to have!

mkitti · 2024-02-27T08:23:18Z

I agree. I just have to find some time to get back around to it. Feel free to beat me to it.

mkitti

Commit suggestions from Tim Holy

Co-authored-by: Tim Holy <[email protected]>

mkitti · 2024-05-06T07:39:59Z

@timholy and @tlnagy I have written tests.

timholy

Thanks for adding the tests and other fixes! It's looking good, but I've left a few other notes about things that seem likely to need fixing.

In cases where you're unsure how something should behave, I tend to expect that mappedarray(complement, a) should be a reasonable guide for how ComplementArray should behave. IF, that is, I understand why it's here at all.

timholy · 2024-05-06T17:04:24Z

src/ColorVectorSpace.jl

+Base.parent(a::ComplementArray) = a.parent
+Base.size(a::ComplementArray) = size(a.parent)
+function Base.getindex(a::ComplementArray{T}, I::Vararg{Int,N})::T where {T,N}
+    getindex(a.parent, I...)


Is this still a TODO? Given that I don't know what this is about, I'm unsure.

timholy · 2024-05-06T17:05:45Z

test/Complement.jl

+        @test red(comp) ≈ red(comp2)
+        @test green(comp) ≈ green(comp2)
+        @test blue(comp) ≈ blue(comp2)
+        @test alpha(comp) == alpha(comp2)


Suggested change

@test alpha(comp) == alpha(comp2)

@test alpha(comp) == alpha(comp2) == alpha(_color)

timholy · 2024-05-06T17:06:25Z

test/Complement.jl

+        @test red(comp) ≈ red(comp2)
+        @test green(comp) ≈ green(comp2)
+        @test blue(comp) ≈ blue(comp2)
+        @test alpha(comp) == alpha(comp2)


Suggested change

@test alpha(comp) == alpha(comp2)

@test alpha(comp) == alpha(comp2) == 1

timholy · 2024-05-06T17:07:29Z

test/Complement.jl

+        @test color_type(comp) == Complement{RGB{Float64}, Float64, 4}
+        @test base_color_type(comp) == Complement{RGB, <:Any, 4}
+        @test base_colorant_type(comp) == Complement{RGBA, <:Any, 4}
+        @test red(comp) ≈ red(comp2)


Any reason these shouldn't be == instead of ≈?

timholy · 2024-05-06T17:07:52Z

test/Complement.jl

+        @test base_color_type(comp) == Complement{Gray, <:Any, 1}
+        @test base_colorant_type(comp) == Complement{Gray, <:Any, 1}
+        @test gray(comp) ≈ gray(comp2)
+        @test alpha(comp) == alpha(comp2)


Suggested change

@test alpha(comp) == alpha(comp2)

@test alpha(comp) == alpha(comp2) == 1

timholy · 2024-05-06T17:19:25Z

src/ColorVectorSpace.jl

+struct ComplementArray{T,N,A<:AbstractArray{<:Complement{T},N}} <: AbstractArray{T,N}
+    parent::A
+    ComplementArray(parent::A) where {T,N,A <: AbstractArray{<:Complement{T},N}} = new{T,N,A}(parent)
+end


Fundamentally I think I don't understand what this is for. I imagine this is the same as ca = mappedarray(complement, a) but without relying on MappedArrays. Is that true, or is it something else?

timholy · 2024-05-06T18:05:13Z

src/ColorVectorSpace.jl

+ColorTypes.color(c::Complement) = Complement(color(parent(c)))
+ColorTypes.color_type(::Type{Complement{C, T, N}}) where {C,T,N} = Complement{color_type(C), T, N}
+ColorTypes.base_color_type(::Type{<: Complement{C, <: Any, N}}) where {N,C} = Complement{base_color_type(C), <: Any, N}
+ColorTypes.base_colorant_type(::Type{<: Complement{C, <: Any, N}}) where {N,C} = Complement{base_colorant_type(C), <: Any, N}


I'm not sure we always want the <:Any on the RHS. Can we also specialize ColorTypes.base_color_type(::Type{Complement{C, T, N}}) where {C,T,N}?

Moreover, since base_color_type(complement(c)) === base_color_type(c), I suspect you want the same property for Complement. I think of Complement as something that represents a lazy computation but it doesn't fundamentally change the underlying colorspace.

timholy · 2024-05-06T18:07:39Z

test/Complement.jl

+            @test reinterpret(typeof(comp), _color) == comp
+            @test reinterpret(Complement, _color) == comp
+        end
+        @test all(reinterpret(typeof(comp), [_color]) .≈ [comp])


Missing:

julia> complement(complement(c)) == c true julia> Complement(Complement(c)) == c false

The latter should fail for === but pass for ==.

timholy · 2024-05-06T18:13:58Z

src/ColorVectorSpace.jl

+function Base.setindex!(a::ComplementArray{T}, v, I::Vararg{Int, N}) where {T,N}
+    setindex!(a.parent, convert(eltype(a.parent), v))
+end
+Base.IndexStyle(a::ComplementArray) = IndexStyle(a.parent)


Given your Vararg{Int, N} for any N I doubt that's really needed.

timholy · 2024-05-06T18:14:33Z

test/Complement.jl

+        comp2 = complement(_color)
+        @test comp isa Complement{RGBA{Float64}, Float64, 4}
+        @test color_type(comp) == Complement{RGB{Float64}, Float64, 4}
+        @test base_color_type(comp) == Complement{RGB, <:Any, 4}


When you're testing types I recommend === (it's nicer on the compiler, for one, and should let the tests run more quickly). Not sure I agree with the choice for the RHS though.

mkitti · 2024-05-06T21:26:41Z

Thanks for adding the tests and other fixes! It's looking good, but I've left a few other notes about things that seem likely to need fixing.

In cases where you're unsure how something should behave, I tend to expect that mappedarray(complement, a) should be a reasonable guide for how ComplementArray should behave. IF, that is, I understand why it's here at all.

mappedarray(complement, a) is a good choice if you have an array of C <: Colorant and want an AbstractArray{C} that has an element type of C.
ComplementArray is for when you have a AbstractArray{Complement} and want a AbstractArray{C} where C <: Colorant. Now that you mention mappedarray, it could probably just be a mappedarray(parent, a).

kimikage · 2024-05-07T11:27:55Z

it could probably just be a mappedarray(parent, a).

or more symmetrically, mappedarray(complement, a)?
(In short, we don't care if it supports Base.parent() or not.)

kimikage · 2024-05-09T00:36:50Z

My main concern is operation and support in the downstream packages.

julia> Complement(RGB(1,1,0)) isa AbstractRGB
false

It is not very pleasant to have to wrap AbstractRGB (or AbstactGray) with Union, in order to support Complement.
One ad hoc solution is to define ComplementRGB{T} <: AbstractRGB{T}, etc., respectively.😑

One idea is to push such an ugly hack into a separate package.
cf. https://github.com/kimikage/StorageColorTypes.jl

Edit:
In other words, my real concern is whether to implement InvertedGray/ComplementGray in ColorTypes.jl at the start point of the discussion.
xref: JuliaGraphics/ColorTypes.jl#295 (comment)

kimikage · 2024-05-09T01:15:42Z

For reference, AbstractGray will be changed from an alias to an abstract type (sometime in the future).
cf. JuliaGraphics/ColorTypes.jl#252
Ironically, the change is to address this issue properly. (JuliaGraphics/ColorTypes.jl#230)

timholy · 2024-05-10T08:24:46Z

The other option is to convert AbstractRGB into a trait. That's feasible but a little harder to work with (given that traits do not have a standard convenient interface) than leaving it as an abstract type.

mkitti · 2024-06-30T05:24:41Z

@tlnagy The outstanding question here is if mappedarray(complement, a) would satisfy the question you put forth in tlnagy/TiffImages.jl#134 . Is that consistent with "Colors.jl promise to faithfully capture the interpretation of the raw data"?

To clearly indicate that I am not intending further action here until a downstream application arises, I am closing this pull request. I would be willing to reopen if a need arises.

Add Complement and Inverse types

d8a4c56

mkitti mentioned this pull request Nov 29, 2023

InvertedGray needed for TIFF MINISWHITE JuliaGraphics/ColorTypes.jl#295

Closed

Forward Complement properties to parent Colorant

984b1c5

timholy reviewed Dec 20, 2023

View reviewed changes

Drop arithmetic and math on Complement

898a7b3

mkitti commented Jan 9, 2024

View reviewed changes

src/ColorVectorSpace.jl Outdated Show resolved Hide resolved

Update src/ColorVectorSpace.jl

ffe30b9

mkitti marked this pull request as ready for review January 9, 2024 19:30

Complement subtypes Color now

0352cbf

mkitti added 3 commits January 10, 2024 04:10

Refactor, implement show

95cb4e6

Resolve ambiguities

e1f9194

Fix ambiguity

c71593a

timholy reviewed Jan 27, 2024

View reviewed changes

mkitti commented Feb 27, 2024

View reviewed changes

mkitti and others added 6 commits February 27, 2024 03:24

Apply suggestions from Tim Holy code review

7245c8b

Co-authored-by: Tim Holy <[email protected]>

Add initial tests for Complement, ComplementArray

e37ab5f

Test on Julia 1.6 and above, fix macOS test

b5ad4ff

Restrict reinterpret of non-arrays to Julia 1.10

9842369

Fix reinterpret

d75b0ea

detect_ambiguities is broken on Julia 1.6

070de6b

mkitti requested a review from timholy May 6, 2024 07:39

timholy reviewed May 6, 2024

View reviewed changes

mkitti closed this Jun 30, 2024

		Mathematical operations on `Complement` will affect the underlying
		`Colorant` rather than changing the value of the complement. That is

	@test alpha(comp) == alpha(comp2)
	@test alpha(comp) == alpha(comp2) == alpha(_color)

	@test alpha(comp) == alpha(comp2)
	@test alpha(comp) == alpha(comp2) == 1

Create a Complement type #189

Create a Complement type #189

Conversation

mkitti commented Nov 29, 2023 • edited Loading

mkitti commented Nov 29, 2023

codecov bot commented Nov 29, 2023 • edited Loading

Codecov Report

mkitti commented Nov 29, 2023

tlnagy commented Dec 6, 2023

timholy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkitti commented Dec 20, 2023

tlnagy commented Jan 9, 2024

mkitti commented Jan 9, 2024

tlnagy commented Jan 9, 2024 • edited Loading

mkitti commented Jan 9, 2024

tlnagy commented Jan 9, 2024

mkitti commented Jan 9, 2024

mkitti commented Jan 10, 2024

tlnagy commented Jan 10, 2024

mkitti commented Jan 10, 2024

mkitti commented Jan 10, 2024

mkitti commented Jan 10, 2024

tlnagy commented Jan 10, 2024 • edited Loading

mkitti commented Jan 24, 2024

tlnagy commented Jan 24, 2024

timholy commented Jan 24, 2024

mkitti commented Jan 25, 2024

mkitti commented Jan 25, 2024

timholy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlnagy commented Feb 27, 2024

mkitti commented Feb 27, 2024

mkitti left a comment

Choose a reason for hiding this comment

mkitti commented May 6, 2024

timholy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkitti commented May 6, 2024

kimikage commented May 7, 2024 • edited Loading

kimikage commented May 9, 2024 • edited Loading

kimikage commented May 9, 2024

timholy commented May 10, 2024

mkitti commented Jun 30, 2024

mkitti commented Nov 29, 2023 •

edited

Loading

codecov bot commented Nov 29, 2023 •

edited

Loading

tlnagy commented Jan 9, 2024 •

edited

Loading

tlnagy commented Jan 10, 2024 •

edited

Loading

kimikage commented May 7, 2024 •

edited

Loading

kimikage commented May 9, 2024 •

edited

Loading