JuliaAI / MLJScientificTypes.jl

Implementation of the MLJ scientific type convention
MIT License
17 stars 6 forks source link

Improve error reporting in coerce for unsupported coercions #39

Closed ablaom closed 3 years ago

ablaom commented 4 years ago
julia> X = (x=1:3,)
(x = 1:3,)

julia> coerce(X, :x=>Textual)
ERROR: MLJScientificTypes.CoercionError("`coerce` is undefined for non-tabular data.")
ablaom commented 4 years ago

There's something funny going on here. The error message implies the method coerce(::Val{:other}, X, a...; kw...) is dispatched (confirmed by stack trace) but this is impossible because trait(X) = :table and not :other as is immediately checked. I ran into this before and believe this mysterious wrong dispatch is related to https://github.com/alan-turing-institute/ScientificTypes.jl/issues/104, and so ultimately to the julia bug https://github.com/JuliaLang/julia/issues/36907 .

So let's revisit this after Julia 1.5.1 is released.

OkonSamuel commented 4 years ago

The behavior is same with julia 1.5.1 release

 julia> X = (x=1:3,)
(x = 1:3,)

julia> coerce(X, :x=>Textual)
ERROR: MLJScientificTypes.CoercionError("`coerce` is undefined for non-tabular data.")

julia> InteractiveUtils.versioninfo()
Julia Version 1.5.1
Commit 697e782ab8 (2020-08-25 20:08 UTC)
Platform Info:
  OS: Linux (x86_64-pc-linux-gnu)
  CPU: Intel(R) Core(TM) i3-5005U CPU @ 2.00GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-9.0.1 (ORCJIT, broadwell)

EDIT But this time around the right method is dispatched as shown by the stack trace coerce(::Val{:table}, X, a...; kw...)

OkonSamuel commented 4 years ago

@ablaom. This issue is because there is currently no defined way of coercing 1:3 to Textual. Defining a fall method should solve this issue. The fallback definition may be in the lines of.

function coerce(v, ::Type{T};
                verbosity::Int=1, tight::Bool=false
                )where T2 <: Union{Missing,Finite}
  throw(ArgumentError("Can't coerce $v which is of scitype $(scitype(v)) to scitype $."*
          "Or a better descriptive error message"))
 return
end
ablaom commented 4 years ago

What if we add

coerce(X::AbstractArray, a; kw...) =
    throw(CoercionError("Coercion to $a not supported for arrays of eltype $(eltype(X)). "))

?

This is ought to cover more cases, no?

OkonSamuel commented 4 years ago

This is ought to cover more cases, no?

no. The first case is more general and should cover all cases. But We can still add more fallbacks like the example you gave above (which would give a specific error message when trying to coerce an array type). This would allow for better error messages at different levels.

OkonSamuel commented 3 years ago

On second thought the example you gave is actually more general since UnitRange <: AbstractArray. I'll open a PR that closes this issue.