Reimplement abs2

[Octogonapus]

This PR #131 removed abs2 because of some ambiguities. Although it's not clear to me what those ambiguities are, I think this is a useful method to have. Is it possible to define an implementation without those prior ambiguities?

[kimikage]

Is this a naming issue? I mean, do you need the overloaded methods of Base.abs2?
Or, is it a problem with the restrictions of alternative representations?

As you may have already read, the abs2 issue is mentioned in issue #126.

[Octogonapus]

do you need the overloaded methods of Base.abs2?

Yes, so for the time being I have added the abs2 methods back for my local project. I just wanted to pose the question in case anyone else had a similar thought.

Thanks for linking me to #126, that helps me understand the ambiguities. I will leave it up to the maintainers to decide if there is a reasonable, general abs2 implementation to be found here.

[kimikage]

Perhaps, if we define abs2 in ColorVectorSpace v0.9 generation, it might be abs2(c) = c ⋅ c. However, this is different from the previous definition for RGB. Also, there is intentionally no definition for TransparentGray and TransparentRGB.

Personally, I think it is better to define it locally (i.e. without overloading Base.abs2) in downstream packages or user code. Using mapreducec, we can generalize the definition.

_abs2(c) = mapreducec(v->float(v)^2, +, 0, c)

cc: @timholy

[johnnychen94]

Not overriding the default abs/abs2 function for colorant is a little bit annoying to write generic functions. For example, in the recent ImageDistances upgrade to support CVS 0.9, I have to override Distances.eval_op for colorants because abs/abs2 are not working.

Said that, I think it's the clearest way for future extension to other definitions/implementations so I'm good with the change; there's no need to bring abs/abs2 back. I do, however, think that we need some more documentation in README.md describing this decision in-depth (and perhaps with some copy-and-paste solutions). The current multiplication/variance is not verbose enough, I think.

[johnnychen94]

gripe: svd(img)

julia> svd(rand(Gray, 32, 32))
ERROR: MethodError: no method matching abs2(::Gray{Float64})
Closest candidates are:
  abs2(::Bool) at bool.jl:80
  abs2(::Real) at number.jl:151
  abs2(::Complex) at complex.jl:265
  ...
Stacktrace:
 [1] eigtype(T::Type)
   @ LinearAlgebra /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.6/LinearAlgebra/src/eigen.jl:302
 [2] svd(A::Matrix{Gray{Float64}}; full::Bool, alg::LinearAlgebra.DivideAndConquer)
   @ LinearAlgebra /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.6/LinearAlgebra/src/svd.jl:157
 [3] svd(A::Matrix{Gray{Float64}})
   @ LinearAlgebra /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.6/LinearAlgebra/src/svd.jl:157
 [4] top-level scope
   @ REPL[8]:1

I think we should consider proposing a abs2(f, x) (and also to abs) method in Base:

abs2(x) = abs2(*, x)
abs2(f, x) = f(x, x)

[johnnychen94]

I think we can at least (safely) bring back abs(::Gray) and abs2(::Gray)?

[kimikage]

One of the annoying things about this problem is that abs has not been removed, and it (i.e. v0.9 implementation) is a channel-wise operation. 😕

ColorVectorSpace.jl/src/ColorVectorSpace.jl

Line 194 in dad1e06

abs(c::MathTypes) = mapc(abs, c)

Even if it is a Gray, it is ambiguous whether abs2(::Gray) returns a Gray or real number.

Edit:
Implementing channel-wise abs2(c::MathType) = mapc(abs2, c) is not a problem for consistency within ColorVectorSpace v0.9. Perhaps svd will work.

However, I doubt that it will match what most users want.

[johnnychen94]

abs2(c::MathType) = mapc(abs2, c)
However, I doubt that it will match what most users want.

This will bring back abs2(c::RGB) with totally different results so I don't think it is a good solution for CVS 0.9.z releases. The best result of this is that we get errors (but it may also return wrong results silently). If I understand your words correctly, you mean exactly the same thing?

[kimikage]

@johnnychen94, do you think that abs should also be removed for consistency?
Or is JuliaImages/Images.jl#955 to improve robustness for the future?

[johnnychen94]

JuliaImages/Images.jl#955 is just a temporary patch. Many of the codes there will soon be deprecated(I'm working on it) and thus removed in the future.

I'm okay to this change but I don't think there's an urgent need for it; abs for Gray is not ambiguous.

If you decide to remove that, since now most packages (in JuliaImages) don't directly rely on CVS, I can patch it appropriately and make it non-breaking in ImageCore side.

[kimikage]

I've always been a big fan of compatibility, so I'm not aggressive about removing abs/abs2.

abs for Gray is not ambiguous.

I think channel-wise abs is one reasonable definition, but I don't think it's the only clear definition.

I'm not going to rush to do anything about abs/abs2. However, I believe @timholy has released ColorVectorSpace v0.9 as a candidate for v1.0. So, the decision will need to be made soon anyway. Also, I would like to reduce the factors of stagnation for ColorTypes/Colors.

[kimikage]

As a first step, I merged PR #164. I leave this issue open.

[timholy]

I'd support having it for Gray. RGB is a much trickier issue.

[johnnychen94]

I realize that people are suffering from this and can't figure out a solution very quickly. Maybe it's a good idea to provide a few non-exported names for compatibility with the old codebase and other ecosystems? For example, provide abs_compat/abs2_compat/norm_compat? I could do the work if you agree on this.

[timholy]

Can anyone who has been suffering from this try the definition abs2(c) = c ⋅ c and see if they're happy with it?

[timholy]

No one took the bait, but I've pushed an implementation in #172. Perhaps folks can give it a try for a bit before we merge.