move implementations into type overloading (aka. functor) #139
Conversation
CorrDist is excluded from `UnionMetrics` since it's a simple wrap on CosineDist
The previous specification is needed to pass a centralized input, now we don't need it anymore
changed parts: * spaces after type annotation, e.g, `b::AbstractMatrix = a` not changed parts: * additional spaces at the end of line * spaces between operations and comma, e.g., `a + b` and `(a, b)`
|
This PR also solves the potential ambiguities Before this PR: julia> foreach(enumerate(detect_ambiguities(Distances))) do (i, item)
println("Methods $i:")
println(item[1])
println(item[2])
end
Methods 1:
evaluate(d::Union{BrayCurtis, Chebyshev, ChiSqDist, Cityblock, CorrDist, CosineDist, GenKLDivergence, Hamming, JSDivergence, Jaccard, KLDivergence, RogersTanimoto, SpanNormDist, TotalVariation, Euclidean, SqEuclidean, Minkowski, RenyiDivergence}, a::Union{SubArray{T,1,Array{T,2},Tuple{Base.Slice{Base.OneTo{Int64}},Int64},true} where T, Array}, b::Union{SubArray{T,1,Array{T,2},Tuple{Base.Slice{Base.OneTo{Int64}},Int64},true} where T, Array}) in Distances at /Users/jc/.julia/packages/Distances/HOWRG/src/metrics.jl:153
evaluate(::CorrDist, a::AbstractArray, b::AbstractArray) in Distances at /Users/jc/.julia/packages/Distances/HOWRG/src/metrics.jl:269After this PR: julia> detect_ambiguities(Distances)
0-element Array{Tuple{Method,Method},1} |
|
FYI, shoving a code formatting commit into a PR that also does non-formatting changes makes it pretty annoying to review. Is it possible to leave out the formatting stuff to its own PR? |
|
But I like the change :). Makes the code simpler in many places. |
|
Ah, I thought you guys would review the codes with the non-whitespace feature provided by github 😂 I can rollback the formatting part if you insist. |
|
Well, I also want to review the whitespace changes separately to see if they make sense :) |
This PR fixes all the ambiguities as a good start, future PRs may not break this.
|
Fortunately, all the format changes are rolled back, and I've added a Since I need changes from this and #140 to continue my work in ImageDistances.jl, could you please tag a release after merging this? |
3 participants
This PR is related to #22, one can easily rename
evaluatelater if we reach a consensus.By moving implementations into functor,
evaluatenow is simply a really thin wrapper here;evaluateis limited only ingeneric.jlnow (line 24).This reduces the possibility of meeting method ambiguity introduced by inconsistent type specification. For example, the implementation of
CorrDistis cleaner.Another change is the new usage
dist(x, y), e.g.,Euclidean()(x, y).P.S.
[1] Vscode auto-formats some codes here and I commit it as well.
[2] For
ImageDistances.jl, it will be much easier to extend toColorantelements without getting lost in method ambiguity and dispatch orders 😄cc: @andreasnoack @KristofferC @ararslan