MLJBase.jl icon indicating copy to clipboard operation
MLJBase.jl copied to clipboard

another function declaration issue

Open nsajko opened this issue 3 years ago • 1 comments

Take a look at these two method definitions: https://github.com/JuliaAI/MLJBase.jl/blob/dev/src/measures/probabilistic.jl#L91-L96

The second method seems faulty in several ways:

  1. all four type parameters are unbound
  2. ArrMissing{UnivariateFinite}, is parameterized with an abstract type. usually one would do something like this instead: ArrMissing{<:UnivariateFinite}
  3. I think the methods are actually ambiguous or overwrite eachother, because there are no constraints on either one

nsajko avatar Sep 09 '22 06:09 nsajko

Thanks for pointing this out.

I am not aware that unbound type parameters matter for performance in method dispatch. Can you explain a bit more about that, or point out a reference?

We might want to throw an error if P is not real - but as a type check, not to address performance. So we could replace the first call with

call(::AUC, ŷ::ArrMissing{UnivariateFinite{S,V,R,P}}, y) where {S,V,R,P<:Real} =
    _auc(P, ŷ, y)

or, the equivalent

call(::AUC, ŷ::ArrMissing{UnivariateFinite{<:Any,<:Any,<:Any}}, y) where P<:Real =
    _auc(P, ŷ, y)

It looks like the second method definition you refer to is trying to address something better solved by implementing an promote_rule for the UnivariateFinite type (defined at CategoricalDistributions.jl). Here's the issue:

using CategoricalDistributions
d1 = UnivariateFinite([0.2, 0.3])
d2 = UnivariateFinite([Float32(0.2), Float32(0.3)]);
v = [d1, d2];
julia> typeof.(v)
2-element Vector{DataType}:
 UnivariateFinite{Multiclass{2}, String, UInt8, Float64}
 UnivariateFinite{Multiclass{2}, String, UInt8, Float32}
julia> eltype(v)
UnivariateFinite{Multiclass{2}, String, UInt8}

So the the first method call will not catch this case. What the last line in the code snippet above "should" be is UnivariateFinite{Multiclass{2}, String, UInt8, Float64}, which a promote_rule "lifting" promotion in P will do, right?

ablaom avatar Sep 12 '22 00:09 ablaom