Skip to content
This repository has been archived by the owner on Jun 22, 2021. It is now read-only.

Improve error reporting in coerce for unsupported coercions #39

Closed
ablaom opened this issue Jun 4, 2020 · 6 comments
Closed

Improve error reporting in coerce for unsupported coercions #39

ablaom opened this issue Jun 4, 2020 · 6 comments

Comments

@ablaom
Copy link
Member

ablaom commented Jun 4, 2020

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

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

ablaom commented Aug 24, 2020

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 JuliaAI/ScientificTypes.jl#104, and so ultimately to the julia bug JuliaLang/julia#36907 .

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

@OkonSamuel
Copy link
Member

OkonSamuel commented Aug 26, 2020

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

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

ablaom commented Aug 26, 2020

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

OkonSamuel commented Aug 26, 2020

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

OkonSamuel commented Dec 18, 2020

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

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

No branches or pull requests

2 participants