Change Mahalanobis input to AbstractMatrix

[rmcaixeta]

This closes #181.
Would be good to accept AbstractMatrix as input to Mahalanobis
Otherwise, it'd not accept a StaticMatrix input
Made a simple adjust and tested here with a StaticMatrix. Working nice

[juliohm]

Could someone please take a look at this PR? We found an application downstream that requires static allocated matrices for performance.

[dkarrasch]

No need to mention M here. Trailing types can be left implicit.

[juliohm]

Should this be modified before the PR can be merged @dkarrasch or it can be left that way? I personally find the explicit version with the 2 type parameters better.

[dkarrasch]

Sure, this is just a style comment. OTOH, it's Julia style not to spell out all parameters. There is one other thing that confuses me. Is it really intentional to have the eltype of the matrix determine the result_type? What if you have an integer matrix, but float vectors? Shouldn't the result_type be sufficiently large to hold one(Ta)*one(T)*one(Tb)[ + one(Ta)*one(T)*one(Tb)]?

[juliohm]

That is a good point. I guess #179 also tries to fix this issue, right? The result type should be a combination of all argument types with promote_type.

[juliohm]

@johnnychen94 since you are up-to-date with the linked issue above, what is your suggestion for the result_type of Mahalanobis here? Can it just be the following?

result_type(::Mahalanobis{T,M}, ::AbstractVector{T1}, ::AbstractVector{T2}) where {T,T1,T2} = reduce(promote_type, (T,T1,T2))

[devmotion]

IMO it is better to not use promote_type but rather a similar approach as in other places currently by defining

result_type(d::SqMahalanobis, ::Type{T1}, ::Type{T2}) where {T1,T2} = typeof(oneunit(eltype(d.qmat)) * (oneunit(T1) - oneunit(T2)))

In contrast to promote_type, this will work with unitful numbers as well.

I also don't think one should worry about a and b being vectors of matrices etc. but rather adopt the convention that the type based definitions of result_type always operate with the element types. Supporting vectors of matrices, colorants, etc. should then be possible by just defining

result_type(d::Metric, a::MyCustomType, b::MyCustomType) = result_type(d, get_the_eltype(a), get_the_eltype(b))

similar to the existing definitions for AbstractArray.

[dkarrasch]

Sounds good. I changed a detail, oneunit(T1) - oneunit(T2), because this is how it's computed, and this may yield a new type (I'm thinking of T1 == T2 == Bool).

[devmotion]

Ah, thanks for noticing! Actually, to correct my statement about unitful numbers, it should be

function result_type(d::SqMahalanobis, ::Type{T1}, ::Type{T2}) where {T1,T2}
    z = oneunit(T1) - oneunit(T2)
    return typeof(z * oneunit(eltype(d.qmat)) * z)
end

since for unitful numbers typeof(z * z) != typeof(z).

[johnnychen94]

When I introduced this approach in #140, my main focus was that method dispatching becomes way too confusing to write correctly, and it was very easy to introduce method ambiguity. For example, JuliaImages/ImageDistances.jl#28 (comment)

Given that Distances.jl computes numerical data, container type (array or colorant) is not needed to infer the result type so there is no need to dispatch on arrays.

[rmcaixeta]

Nice. @dkarrasch , PR updated with the 2nd correction:

function result_type(d::Mahalanobis, ::Type{T1}, ::Type{T2}) where {T1,T2}
    z = oneunit(T1) - oneunit(T2)
    return typeof(z * oneunit(eltype(d.qmat)) * z)
end

function result_type(d::SqMahalanobis, ::Type{T1}, ::Type{T2}) where {T1,T2}
    z = oneunit(T1) - oneunit(T2)
    return typeof(z * oneunit(eltype(d.qmat)) * z)
end

[dkarrasch]

Very nice @rmcaixeta! If you could change the version number in the Project.toml file to "0.10.0", then I can merge and release right away. 🎉

[rmcaixeta]

Great. Thanks everyone!

[juliohm]

Thank you guys! Looking forward to having this merged and tagged. We have a PR downstream waiting for these changes. 👍

[dkarrasch]

Sorry, again, could you please resolve the merge conflict. I don't want to mess up the commits.

[johnnychen94]

Never experienced such kind of discussion & collaboartion outside of the Julia community ❤️

[rmcaixeta]

Sorry, again, could you please resolve the merge conflict. I don't want to mess up the commits.

Sure. Guess now it's okay

[nalimilan]

It would have been nice to add a test, otherwise StaticMatrix support can be broken at any time without anybody noticing. Also now non-Real inputs are allowed, but this isn't tested either.