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.

Sign up for GitHub

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

Jump to bottom

Bug/move std to cvs #125

Closed
wants to merge 2 commits into from
Closed

Bug/move std to cvs #125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
26d940a
Add var, std, complement. Migrated from Images.jl
wizofe Mar 7, 2020
e6041e0
Add test for complement. Migrated from Images.jl
wizofe Mar 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 31 additions & 1 deletion src/ColorVectorSpace.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@wizofe wizofe Mar 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Copy link
Member

@johnnychen94 johnnychen94 Mar 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:/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)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@johnnychen94 johnnychen94 Mar 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@johnnychen94 johnnychen94 Mar 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Collaborator

@kimikage kimikage Mar 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Statistics module does not fully support colors.
  2. 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.

Copy link
Member

@johnnychen94 johnnychen94 Mar 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@kimikage kimikage Mar 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 😈

base_colorant_type(C)(var(imgc; kwargs...))
end

function var(A::AbstractArray{C,N}; kwargs...) where {C<:Colorant,N}
imgc = channelview(A)
colons = ntuple(d->Colon(), Val(N))
inds1 = axes(imgc, 1)
val1 = var(view(imgc, first(inds1), colons...); kwargs...)
vals = similar(imgc, typeof(val1), inds1)
vals[1] = val1
for i in first(inds1)+1:last(inds1)
vals[i] = var(view(imgc, i, colons...); kwargs...)
end
base_colorant_type(C)(vals...)
end

"""
y = complement(x)

Take the complement `1-x` of `x`. If `x` is a color with an alpha channel,
the alpha channel is left untouched. Don't forget to add a dot when `x` is
an array: `complement.(x)`
"""
complement(x::Union{Number,Colorant}) = oneunit(x)-x
complement(x::TransparentColor) = typeof(x)(complement(color(x)), alpha(x))

std(A::AbstractArray{C}; kwargs...) where {C<:Colorant} = mapc(sqrt, var(A; kwargs...))
middle(c::AbstractGray) = arith_colorant_type(c)(middle(gray(c)))
middle(x::C, y::C) where {C<:AbstractGray} = arith_colorant_type(C)(middle(gray(x), gray(y)))

Expand Down Expand Up @@ -343,4 +372,5 @@ _precompile_()
@deprecate (*)(A::AbstractArray{T}, b::TransparentGray) where {T<:Number} A.*b
@deprecate (*)(b::TransparentGray, A::AbstractArray{T}) where {T<:Number} A.*b

@deprecate complement(x::AbstractArray) complement.(x)
end
15 changes: 15 additions & 0 deletions test/runtests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# Complement
@test complement(Gray(0.5)) == Gray(0.5)
@test complement(Gray(0.2)) == Gray(0.8)
@test all(complement.(img) .== 1 .- img)
# deprecated (#690)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@test all(complement.(img) .== 1 .- img)
end

@testset "Comparisons with Gray" begin
Expand Down Expand Up @@ -276,6 +283,14 @@ end
@test RGB24(1,0,0)/2.0 === RGB(0.5,0,0)
end

@testset "Complement" begin
@test complement.([Gray(0.2)]) == [Gray(0.8)]
@test complement.([Gray{N0f8}(0.2)]) == [Gray{N0f8}(0.8)]
@test complement.([RGB(0,0.3,1)]) == [RGB(1,0.7,0)]
@test complement.([RGBA(0,0.3,1,0.7)]) == [RGBA(1.0,0.7,0.0,0.7)]
@test complement.([RGBA{N0f8}(0,0.6,1,0.7)]) == [RGBA{N0f8}(1.0,0.4,0.0,0.7)]
end

@testset "Arithemtic with RGBA" begin
cf = RGBA{Float32}(0.1,0.2,0.3,0.4)
@test @inferred(zero(cf)) === RGBA{Float32}(0,0,0,0)
Expand Down