Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

another function declaration issue #835

Open
nsajko opened this issue Sep 9, 2022 · 1 comment
Open

another function declaration issue #835

nsajko opened this issue Sep 9, 2022 · 1 comment

Comments

@nsajko
Copy link
Contributor

nsajko commented Sep 9, 2022

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
@ablaom
Copy link
Member

ablaom commented Sep 12, 2022

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants