Bug/move std to cvs #125

wizofe · 2020-03-07T20:55:53Z

Migrate mean, std and complement from JuliaImages.jl including tests.

johnnychen94

@wizofe thank you so much working on this, it looks pretty good

But we still need some discussion on the ImageCore issue (for now you can just add it to let the test pass :P)

johnnychen94 · 2020-03-08T05:03:45Z

test/runtests.jl

+        @test complement(Gray(0.5)) == Gray(0.5)
+        @test complement(Gray(0.2)) == Gray(0.8)
+        @test all(complement.(img) .== 1 .- img)
+        # deprecated (#690)


the link is invalid now, needs update to JuliaImages/Images.jl#690

johnnychen94 · 2020-03-08T05:04:44Z

src/ColorVectorSpace.jl

@@ -10,7 +10,7 @@ import Base: abs, abs2, clamp, convert, copy, div, eps, isfinite, isinf,
 import LinearAlgebra: norm
 import StatsBase: histrange, varm
 import SpecialFunctions: gamma, lgamma, lfact
-import Statistics: middle
+import Statistics: middle, var, std

 export nan


complement needs to be exported so that users don't need to write ColorVectorSpaces.complement

Would that be as simple as export complement in that case?

johnnychen94 · 2020-03-08T05:10:31Z

test/runtests.jl

@@ -126,6 +126,13 @@ end
        @test Gray24(0.8)*0.5 === Gray(0.4)
        @test Gray24(0.8)/2   === Gray(0.5f0*N0f8(0.8))
        @test Gray24(0.8)/2.0 === Gray(0.4)
+


The failed std tests in JuliaImages/Images.jl#879 need to move here. But still, leave a copy there so that Images.jl behaves the same to end-users.

Could you also add some tests for var? There should be but looks like we didn't do it in Images.jl

johnnychen94 · 2020-03-08T05:27:27Z

src/ColorVectorSpace.jl

@@ -207,6 +207,35 @@ for op in unaryOps
    @eval ($op)(c::AbstractGray) = $op(gray(c))
 end

+function var(A::AbstractArray{C}; kwargs...) where C<:AbstractGray
+    imgc = channelview(A)


Oops, channelview comes from ImageCore, which is so unfortunate because we might not want to introduce ImageCore as a dependency on ColorVectorSpace; instead, we prefer using ColorVectorSpace in ImageCore. JuliaGraphics/ColorTypes.jl#160 (comment)

But given that the best place I have in mind for var is ColorVectorSpace, adding ImageCore to ColorVectorSpace seems a must choice.

cc: @timholy

I don't think we need the channelview here just to implement var.
However, if there are many such methods using channelview, I think that the versions using channelview should be extended in ImageCore.jl.

BTW, I do not fully understand why var of vectors should be element-wise (channel-wise).:sweat_smile:

I don't think we need the channelview here just to implement var.

No, we don't necessarily need, but it would make trivial things less trivial. Another choice is to move these functions to ImageDistances, rename it to ImageStats and absorb ImageQualityIndexes as well.

I do not fully understand why var of vectors should be element-wise (channel-wise)

Generally, for non-gray image x we don't know how to combine those channels, so doing it channelwisely is saying "oh man I don't know what you really want, this is all I can do without surprising you". But yes, conventionally RGB image can be one special case that most people just treat it as 3*m*n array.

For example, PSNR treats RGB and other Color3 images differently. https://github.com/JuliaImages/ImageQualityIndexes.jl/blob/master/src/psnr.jl#L13-L21

From wikipedia

Alternately, for color images the image is converted to a different color space and PSNR is reported against each channel of that color space, e.g, YCbCr or HSL

I agree that we may need one more method for AbstractRGB images.

It's a little inconsistent, but ImageDistances doesn't split channels even for RGB images
JuliaImages/ImageDistances.jl#16 (comment)

IMO, if there are no clear rules about combining channels, ColorVectorSpace should only support a single-channel (grayscale) arrays.

Basically I agree with you. From the commit history I think deprecating it would be acceptable.

But at least for now the best choice is to move it here and to add a deprecation warning.

My suggestion is to keep AbstractGray and AbstractRGB and to throw depwarn for other Colorant types unless there's a specific usage for them. Does this sound an acceptable solution? @kimikage @timholy

You may think my comments are inconsistent, but I'm not negative in channel-wise variance itself. I just care about the problem of a lower-level package depending on higher-level packages.
By analogy with the multivariate statistics, I think the current behavior is reasonable. So I don't think we need the deprecation.

julia> using Statistics julia> wbb = [[1.0,1.0,1.0], [0.0,0.0,0.0], [0.0,0.0,0.0]] 3-element Array{Array{Float64,1},1}: [1.0, 1.0, 1.0] [0.0, 0.0, 0.0] [0.0, 0.0, 0.0] julia> rgb = [[1.0,0.0,0.0], [0.0,1.0,0.0], [0.0,0.0,1.0]] 3-element Array{Array{Float64,1},1}: [1.0, 0.0, 0.0] [0.0, 1.0, 0.0] [0.0, 0.0, 1.0] julia> var(wbb) 3-element Array{Float64,1}: 0.33333333333333337 0.33333333333333337 0.33333333333333337 julia> var(rgb) 3-element Array{Float64,1}: 0.33333333333333337 0.33333333333333337 0.33333333333333337

I think the current behavior is reasonable

It makes sense. I had a feeling about that but didn't check that previously.

Corrects me if I don't understand it right, let me try to state the roles of ImageCore, Colors and ColorVectorSpaces

Colors provides fundamental color operations on pixel-level as a delicately designed art.

ColorVectorSpaces provides extensive glue support on pixel-level for methods from other packages, mostly Base.

ImageCore is like Colors but it provides operations on array-level.

Revisiting ColorVectorSpaces, I don't see any method on Array types.

I now think I was wrong saying ColorVectorSpace is the most suitable place for var and std in JuliaImages/Images.jl#872 So for now, they may still stay in Images.

For the array-level glue package, we only have ImageDistances and ImageAxes, but we don't have one that glues StatsBase.

For this PR, one thing for sure is that complement can stay in this package.

But...I now think it might be better to stay in Colors...

I agree for the most part of the roles.
However, I think the role of ColorVectorSpace, as the name implies, is to define a vector space for colors, i.e. to introduce linearity. The introduction of linearity allows for the addition, subtraction and uniform scaling of colors. So we can define the "mean" of the colors. In fact, we haven't defined the mean method in ColorVectorSpace, but the Statistics module provides it.

Similarly, var and std can be defined systematically. However, there are two problems:

Statistics module does not fully support colors.

The definition of the "difference" from the mean color is ambiguous.

My comment above

BTW, I do not fully understand why var of vectors should be element-wise (channel-wise).

means "2.". I don't think "pixel-level" or "array-level" is the essence of the problem. Therefore, I think that defining "default" (fallback) var in ColorVectorSpace itself is fine, but I disagree with using channelview just for that.

For this PR, one thing for sure is that complement can stay in this package.

But...I now think it might be better to stay in Colors...

The current complement implicitly assumes linearity. For example, what is the "complement" of Lab(20, 30, 40)? The complementary color is a more general (i.e. ambiguous) concept.
Of course, I'm not against defining a more generalized complement in Colors.jl. IMO, the complementary color should be defined by "neutral gray", not white. In other words, I think Lab(80, -30, -40) is appropriate for the complement(Lab(20,30,40)).
However, although the XYZ space is designed to have linearity "for convenience", it is difficult to define the complementary colors in the XYZ space. Perhaps we need an additional argument whitepoint to define the neutral gray. So, the discussion is needed.

I think the role of ColorVectorSpace, as the name implies, is to define a vector space for colors, i.e. to introduce linearity.

Yes, I understand this is the motivation of this package. Still, I tried to avoid saying this when I described the roles because I thought it doesn't help to solve the current issue.

For example, by saying this, any function in ImageDistances has their right to stay here, but introducing any of them would trigger such a discussion thread.

Therefore, I think that defining "default" (fallback) var in ColorVectorSpace itself is fine, but I disagree with using channelview just for that.

The pixel/array level difference is the only thing that I can speak of the low/high level abstraction. Since both packages don't strictly rely on each other, we can't say that one is more fundamental than the other.

As I said, not using channelview would make trivial things less trivial, it's not very replicable. If we can't use channelview, then IMO it basically suggests either that ColorVectorSpace isn't a good place for var or that channelview should be moved to Colors.jl. However, the status quo is that channelview should still stay in ImageCore.

Thus, I prefer to create an ImageBase package to fit these kinds of needs for array-level operations. Indeed, if there's no objection about that, I can do it myself.

For example, by saying this, any function in ImageDistances has their right to stay here, but introducing any of them would trigger such a discussion thread.

The difference from Statistics is that Distance is not a stdlib. I do not object to creating a new package to reduce the size of the package. (I think ImageCore and ImageBase can be confusing, though.)

The pixel/array level difference is the only thing that I can speak of the low/high level abstraction.

ColorVectorSpace defines the color arithmetic. Perhaps the "pixel-level" you said might mean that, but the arithmetic should be independent of the shape of arrays. In short, the problem with var is a matter of what the type/value of "square error of colors" should be.

As I said, not using channelview would make trivial things less trivial, it's not very replicable.

What you said is correct, but no channel splitter is needed to calculate channel-wise var. Essentially, we need the following methods:

mapreduce(color->mapc(abs2, color - mean_color), +, A)

Handling the keyword arguments (especially dims) is annoying, but I think it's better than depending on the inside of Statistics.jl. Although I have no strong opinion on the location of var, we may obtain the "speed" as a practical effect by not using channelview. 😈

timholy · 2020-03-12T15:20:57Z

I'm writing a looong issue that will touch on this. But one "leftover" from that issue (and hinted at in #125 (comment)) is that our current implementation is buggy if you use the dims keyword of var:

julia> A = rand(RGB{Float32}, 5, 5)
5×5 Array{RGB{Float32},2} with eltype RGB{Float32}:
 RGB{Float32}(0.39131,0.756341,0.550929)    …  RGB{Float32}(0.755703,0.966161,0.095723)
 RGB{Float32}(0.835037,0.614648,0.913182)      RGB{Float32}(0.56782,0.799695,0.421811) 
 RGB{Float32}(0.933872,0.775016,0.694558)      RGB{Float32}(0.700192,0.707239,0.487109)
 RGB{Float32}(0.198452,0.0549943,0.186213)     RGB{Float32}(0.772236,0.840592,0.994962)
 RGB{Float32}(0.869468,0.781943,0.839985)      RGB{Float32}(0.372748,0.603654,0.176182)

julia> var(A; dims=2)
ERROR: MethodError: no method matching RGB(::Array{Float32,2}, ::Array{Float32,2}, ::Array{Float32,2})
Closest candidates are:
  RGB(::Any) at /home/tim/.julia/packages/ColorTypes/RS4tc/src/types.jl:445
Stacktrace:
 [1] #var#2(::Base.Iterators.Pairs{Symbol,Int64,Tuple{Symbol},NamedTuple{(:dims,),Tuple{Int64}}}, ::typeof(var), ::Array{RGB{Float32},2}) at /home/tim/.julia/packages/Images/RUVAg/src/algorithms.jl:94
 [2] (::Statistics.var"#kw##var")(::NamedTuple{(:dims,),Tuple{Int64}}, ::typeof(var), ::Array{RGB{Float32},2}) at ./none:0
 [3] top-level scope at REPL[18]:1

@wizofe, obviously this is not to blame you since you're just moving code around. Wait a bit until I post my in-progress issue for more comments.

timholy · 2021-01-21T22:20:09Z

Superseded by #131. Thanks again @wizofe, sorry we didn't use your contribution!

kimikage · 2021-03-04T02:38:24Z

@timholy, there's also stuff about complement in this PR. It should have been included in v0.9.0, but I realized that too late.
There is still a chance, since Images.jl does not yet support ColorVectorSpace v0.9, though.

I will reopen this PR as a reminder until a new separate PR is submitted.

kimikage · 2021-03-04T08:38:51Z

new PR #144

wizofe added 2 commits March 7, 2020 20:53

Add var, std, complement. Migrated from Images.jl

26d940a

Add test for complement. Migrated from Images.jl

e6041e0

wizofe mentioned this pull request Mar 7, 2020

Shouldn't complement live in Colors.jl? JuliaImages/Images.jl#698

Closed

johnnychen94 mentioned this pull request Mar 8, 2020

Bug/move std to cvs JuliaImages/Images.jl#879

Closed

johnnychen94 reviewed Mar 8, 2020

View reviewed changes

This was referenced Mar 9, 2020

move var, std to ColorVectorSpace JuliaImages/Images.jl#872

Open

Version 0.10.0 JuliaGraphics/ColorTypes.jl#176

Merged

timholy mentioned this pull request Mar 12, 2020

Important decisions with respect to color math (please comment) #126

Closed

timholy closed this Jan 21, 2021

kimikage reopened this Mar 4, 2021

kimikage mentioned this pull request Mar 4, 2021

Add complement migrated from Images.jl #144

Merged

kimikage closed this Mar 4, 2021

kimikage mentioned this pull request Jun 18, 2021

is inv on Colors well defined? JuliaGraphics/Colors.jl#488

Closed

Bug/move std to cvs #125

Bug/move std to cvs #125

Uh oh!

Conversation

wizofe commented Mar 7, 2020

Uh oh!

johnnychen94 left a comment

Choose a reason for hiding this comment

Uh oh!

johnnychen94 Mar 8, 2020

Choose a reason for hiding this comment

Uh oh!

johnnychen94 Mar 8, 2020

Choose a reason for hiding this comment

Uh oh!

wizofe Mar 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnnychen94 Mar 8, 2020

Choose a reason for hiding this comment

Uh oh!

johnnychen94 Mar 8, 2020

Choose a reason for hiding this comment

Uh oh!

kimikage Mar 8, 2020

Choose a reason for hiding this comment

Uh oh!

johnnychen94 Mar 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kimikage Mar 8, 2020

Choose a reason for hiding this comment

Uh oh!

johnnychen94 Mar 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kimikage Mar 8, 2020

Choose a reason for hiding this comment

Uh oh!

johnnychen94 Mar 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kimikage Mar 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnnychen94 Mar 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kimikage Mar 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timholy commented Mar 12, 2020

Uh oh!

timholy commented Jan 21, 2021

Uh oh!

kimikage commented Mar 4, 2021

Uh oh!

kimikage commented Mar 4, 2021

Uh oh!

Uh oh!

wizofe Mar 10, 2020 •

edited

Loading

johnnychen94 Mar 8, 2020 •

edited

Loading

johnnychen94 Mar 8, 2020 •

edited

Loading

johnnychen94 Mar 8, 2020 •

edited

Loading

kimikage Mar 9, 2020 •

edited

Loading

johnnychen94 Mar 9, 2020 •

edited

Loading

kimikage Mar 9, 2020 •

edited

Loading