avoid type piracy?

[johnnychen94]

To follow the same structure in Distances.jl, I override some methods: evaluate, result_type

https://github.com/JuliaImages/ImageDistances.jl/blob/78322bf4cf08b4d61b56763908fcec5cab6f14c7/src/metrics.jl

However, it requires more efforts and codes to make the function works for image inputs, also makes the codes more prone to bugs.

Can we create another name(e.g., measure) to avoid type piracy? Or is there any better alternative strategy?

[juliohm]

Hi Johnny,

Let me try to recap the latest changes. Before we had an independent evaluate function in ImageDistances.jl and so this signature issue didn't exist. On the other hand we couldn't call distances in Distances.jl directly. You then proposed to add methods to the existing evaluate of Distances.jl instead of creating an independent evaluate function in ImageDistances.jl.

Could you clarify the issue now? What is the extra effort you mentioned? Maybe you could give an example with code of the usage you have in mind? Sorry I am not following the development closely, really busy times around here as you know. I am heading to give a lecture now, but I will be back in the afternoon to check the issue.

The API I expect without looking into the code is:

evaluate(dist, A, B) # calls DIstances.jl if A and B are arbitrary 1D arrays
evaluate(dist, imgA, imgB) # calls ImageDistances.jl if arrays are 2D or higher dimension
colwise(dist, A, B) # calls Distances.jl if A and B are 2D arrays of primitive types
colwise(dist, imgsA, imgsB) # calls ImageDistances.jl if imgsA and imgsB are lists of arrays
pairwise(dist, ...) # similar to colwise

Sorry if I missed something, typing at the speed of light here...

[johnnychen94]

The current API works perfectly as you expected.

The recent upgrades in ImageDistances.jl are:

1. replace Image*Metric types with *Metric
2. change the dispatch priority, promote FixedPoint and Bool types to AbstractFloat first, and then feed these into the same functions in Distances.jl. E.g., Array{Gray{N0f8}} --> Array{Float32} --> Distances.jl functions
3. enhance the methods to cover different image using case.

The tricky part is "changing dispatch priority", the major problem is the following three methods in Distances.jl

evaluate(dist::PreMetric, a::AbstractArray, b::AbstractArray)
evaluate(dist, a::AbstractArray, b::AbstractArray)
evaluate(d::UnionMetrics, a::Union{Array, ArraySlice}, b::Union{Array, ArraySlice})

Actually Distances.jl computes AbstractArray{<:Number}, so I have to write some extra redundant codes to fix the dispatching order and ambiguity. The whole metrics.jl file is for this purpose. Besides this, there're more ambiguity fixes:

https://github.com/JuliaImages/ImageDistances.jl/blob/8e24ae1a7ee54997b5d939fd9f18dd1a4a9985a7/src/generic.jl#L86-L93

These two methods additionally introduce new ambiguities to image-specific metrics, e.g.,:

https://github.com/JuliaImages/ImageDistances.jl/blob/8e24ae1a7ee54997b5d939fd9f18dd1a4a9985a7/src/ciede2000.jl#L36-L43

[johnnychen94]

The major problem of previous design, besides additional types, is method conflicts. As long as we take the same evaluate and Distances.jl doesn't restrict its type to Number, implementing new methods would be not so elegant.

(P.S. I opened a PR in Distances.jl but one of its members has some sort of concerns on restriction https://github.com/JuliaStats/Distances.jl/pull/128)

# ImageDistances@0.2.0 # before redesign
julia> using Distances

julia> using ImageDistances
[ Info: Recompiling stale cache file /Users/jc/.julia/compiled/v1.1/ImageDistances/lg0Xg.ji for ImageDistances [51556ac3-7006-55f5-8cb3-34580c88182d]

julia> evaluate
WARNING: both ImageDistances and Distances export "evaluate"; uses of it in module Main must be qualified
ERROR: UndefVarError: evaluate not defined

# ImageDistances@0.2.3 # after redesign
julia> using ImageDistances

julia> using Distances

julia> evaluate
evaluate (generic function with 28 methods)

[nalimilan]

I don't think there's a problem. Yes, it's a bit verbose, but there's no type piracy since you own the types used for dispatch.

[johnnychen94]

Thanks for the clarification, I feel much better not get involved in type piracy of Distances.jl.

[tlnagy]

This package breaks some of my code that uses both Distances.jl and Images:

julia> using Distances

julia> evaluate(WeightedEuclidean([1,1,1000]), [1,2,3], [1,2,4])
31.622776601683793

julia> sqrt(1000)
31.622776601683793

julia> using Images

julia> evaluate(WeightedEuclidean([1,1,1000]), [1,2,3], [1,2,4])
ERROR: MethodError: Distances.result_type(::WeightedEuclidean{Array{Int64,1}}, ::Array{Int64,1}, ::Array{Int64,1}) is ambiguous. Candidates:
  result_type(dist::PreMetric, ::AbstractArray{#s19,N} where N where #s19<:Union{Colorant{T1,N} where N, AbstractArray{#s12,N} where #s12<:Union{Colorant{T1,N} where N, T1} where N, T1}, ::AbstractArray{#s17,N} where N where #s17<:Union{Colorant{T2,N} where N, AbstractArray{#s12,N} where #s12<:Union{Colorant{T2,N} where N, T2} where N, T2}) where {T1<:Number, T2<:Number} in ImageDistances at /home/tamas/.julia/packages/ImageDistances/okX3M/src/generic.jl:83
  result_type(dist::Union{WeightedCityblock{W}, WeightedEuclidean{W}, WeightedHamming{W}, WeightedSqEuclidean{W}, WeightedMinkowski{W,T} where T<:Real} where W, ::AbstractArray{T1,N} where N, ::AbstractArray{T2,N} where N) where {T1, T2} in Distances at /home/tamas/.julia/packages/Distances/HOWRG/src/wmetrics.jl:46
Possible fix, define
  result_type(::Union{WeightedCityblock{W}, WeightedEuclidean{W}, WeightedHamming{W}, WeightedSqEuclidean{W}, WeightedMinkowski{W,T} where T<:Real} where W, ::AbstractArray{T1<:(Union{Colorant{T1,N} where N, AbstractArray{#s12,N} where #s12<:Union{Colorant{T1,N} where N, T1} where N, T1} where T1<:Number),N} where N, ::AbstractArray{T2<:(Union{Colorant{T2,N} where N, AbstractArray{#s12,N} where #s12<:Union{Colorant{T2,N} where N, T2} where N, T2} where T2<:Number),N} where N)
Stacktrace:
 [1] evaluate(::WeightedEuclidean{Array{Int64,1}}, ::Array{Int64,1}, ::Array{Int64,1}) at /home/tamas/.julia/packages/Distances/HOWRG/src/wmetrics.jl:0
 [2] top-level scope at none:0

[timholy]

This package could use a test like this one to ensure that ambiguities are not accidentally introduced. (This is not really piracy, because the intention was to extend a method for Colorants.) This ambiguity can presumably be fixed by defining that method only on Colorant types and not on Union{Colorant,Number} types.