Repository: JuliaAI/MLJBase.jl (julia)

Issue:
Typos in `Stack` constructor lead to uninformative error messages
Here `acceleration` kwargs has been misspelled, so constructor thinks its the name of a base model:

```julia
julia> stack = Stack(
       resampling=CV(nfolds=3),
       accleration=CPUThreads(),
       metalearner = forest,
       regressor1=tree_booster,
       regressor2=svm,
       )
ERROR: MethodError: Cannot `convert` an object of type CPUThreads{Nothing} to an object of type Supervised                                                            
Closest candidates are:
  convert(::Type{T}, ::T) where T at /Applications/Julia-1.7.app/Contents/Resources/julia/share/julia/base/essentials.jl:218
Stacktrace:
  [1] setindex!(A::Vector{Supervised}, x::CPUThreads{Nothing}, i1::Int64)
    @ Base ./array.jl:903
  [2] _unsafe_copyto!(dest::Vector{Supervised}, doffs::Int64, src::Vector{Any}, soffs::Int64, n::Int64)                                                                            
    @ Base ./array.jl:253
  [3] unsafe_copyto!
    @ ./array.jl:307 [inlined]
  [4] _copyto_impl!
    @ ./array.jl:331 [inlined]
  [5] copyto!
    @ ./array.jl:317 [inlined]
  [6] copyto!
    @ ./array.jl:343 [inlined]
  [7] copyto_axcheck!
    @ ./abstractarray.jl:1104 [inlined]
  [8] Vector{Supervised}(x::Vector{Any})
    @ Base ./array.jl:563
  [9] convert
    @ ./array.jl:554 [inlined]
 [10] MLJBase.DeterministicStack(modelnames::Tuple{Symbol, Symbol, Symbol}, models::Vector{Any}, metalearner::MLJDecisionTreeInterface.RandomForestRegressor, resampling::CV, measures::
Nothing, cache::Bool, acceleration::CPU1{Nothing})                                         
    @ MLJBase ~/.julia/packages/MLJBase/jcOVb/src/composition/models/stacking.jl:38
 [11] (Stack)(; metalearner::MLJDecisionTreeInterface.RandomForestRegressor, resampling::CV, measure::Nothing, measures::Nothing, cache::Bool, acceleration::CPU1{Nothing}, named_models
::Base.Pairs{Symbol, Any, Tuple{Symbol, Symbol, Symbol}, NamedTuple{(:accleration, :regressor1, :regressor2), Tuple{CPUThreads{Nothing}, EvoTrees.EvoTreeRegressor{Float64, EvoTrees.Linear, Int64}, MLJLIBSVMInterface.EpsilonSVR}}})
    @ MLJBase ~/.julia/packages/MLJBase/jcOVb/src/composition/models/stacking.jl:171
 [12] top-level scope
    @ REPL[20]:1
 [13] top-level scope
    @ ~/.julia/packages/CUDA/tTK8Y/src/initialization.jl:52
```

Pull request description:
Closes #796

Patch:
diff --git a/src/MLJBase.jl b/src/MLJBase.jl
index 0b220a54..460420fa 100644
--- a/src/MLJBase.jl
+++ b/src/MLJBase.jl
@@ -194,6 +194,7 @@ include("show.jl")
 include("interface/data_utils.jl")
 include("interface/model_api.jl")
 
+include("models.jl")
 include("sources.jl")
 include("machines.jl")
 
diff --git a/src/composition/models/stacking.jl b/src/composition/models/stacking.jl
index 48dd34f3..eecb445a 100644
--- a/src/composition/models/stacking.jl
+++ b/src/composition/models/stacking.jl
@@ -1,8 +1,7 @@
 ############################################
-################ Structures ################
+################ Helpers ###################
 ############################################
 
-
 function glb(types...)
     # If a lower bound is in the types then it is greatest
     # else we just return Unknown for now
@@ -26,6 +25,19 @@ function input_target_scitypes(models, metalearner)
 end
 
 
+############################################
+################ Structures ################
+############################################
+
+const ERR_BAD_METALEARNER = ArgumentError(
+    "The metalearner should be a subtype "*
+    "of $(Union{Deterministic, Probabilistic})"
+)
+
+const ERR_NO_METALEARNER = ArgumentError(
+    "No metalearner specified. Use Stack(metalearner=..., model1=..., model2=...)"
+)
+
 mutable struct DeterministicStack{
     modelnames,
     inp_scitype,
@@ -47,6 +59,9 @@ mutable struct DeterministicStack{
         cache,
         acceleration
     )
+        map(models) do m
+            check_ismodel(m, spelling=true)
+        end
         inp_scitype, tg_scitype = input_target_scitypes(models, metalearner)
         return new{modelnames, inp_scitype, tg_scitype}(
             models,
@@ -80,7 +95,9 @@ mutable struct ProbabilisticStack{
         cache,
         acceleration
     )
-
+        map(models) do m
+            check_ismodel(m, spelling=true)
+        end
         inp_scitype, tg_scitype = input_target_scitypes(models, metalearner)
         return new{modelnames, inp_scitype, tg_scitype}(
             models,
@@ -208,8 +225,7 @@ function Stack(
     named_models...
 )
 
-    metalearner === nothing &&
-        throw(ArgumentError("No metalearner specified. Use Stack(metalearner=...)"))
+    metalearner === nothing && throw(ERR_NO_METALEARNER)
 
     nt = NamedTuple(named_models)
     modelnames = keys(nt)
@@ -218,6 +234,7 @@ function Stack(
         measures = [measures, ]
     end
 
+    check_ismodel(metalearner)
     if metalearner isa Deterministic
         stack =  DeterministicStack(
             modelnames,
@@ -239,8 +256,7 @@ function Stack(
             acceleration,
         )
     else
-        throw(ArgumentError("The metalearner should be a subtype
-                    of $(Union{Deterministic, Probabilistic})"))
+        throw(ERR_BAD_METALEARNER)
     end
     # Issuing clean! statement
     message = MMI.clean!(stack)
diff --git a/src/machines.jl b/src/machines.jl
index f6781985..ddca0ae0 100644
--- a/src/machines.jl
+++ b/src/machines.jl
@@ -140,10 +140,10 @@ err_length_mismatch(model) = DimensionMismatch(
     "Differing number of observations "*
     "in input and target. ")
 
-check(model::Any, args...) =
-    throw(ArgumentError("Expected a `Model` instance, got $model. "))
 function check(model::Model, scitype_check_level, args...)
 
+    check_ismodel(model)
+
     is_okay = true
 
     scitype_check_level >= 1 || return is_okay
@@ -827,8 +827,8 @@ end
 """
     feature_importances(mach::Machine)
 
-Return a list of `feature => importance` pairs for a fitted machine, 
-`mach`,  if supported by the underlying model, i.e., if 
+Return a list of `feature => importance` pairs for a fitted machine,
+`mach`,  if supported by the underlying model, i.e., if
 `reports_feature_importances(mach.model) == true`.  Otherwise return
 `nothing`.
 
diff --git a/src/models.jl b/src/models.jl
new file mode 100644
index 00000000..8e3ad40b
--- /dev/null
+++ b/src/models.jl
@@ -0,0 +1,27 @@
+# # EXCEPETIONS
+
+function message_expecting_model(X; spelling=false)
+    message = "Expected a model instance but got `$X`"
+    if X isa Type{<:Model}
+        message *= ", which is a model *type*"
+    else
+        spelling && (message *= ". Perhaps you misspelled a keyword argument")
+    end
+    message *= ". "
+    return message
+end
+
+err_expecting_model(X; kwargs...) = ArgumentError(message_expecting_model(X; kwargs...))
+
+"""
+    MLJBase.check_ismodel(model; spelling=false)
+
+Return `nothing` if `model` is a model, throwing `err_expecting_model(X; spelling)`
+otherwise. Specify `spelling=true` if there is a chance that user mispelled a keyword
+argument that is being interpreted as a model.
+
+**Private method.**
+
+"""
+check_ismodel(model; kwargs...) = model isa Model ? nothing :
+    throw(err_expecting_model(model; kwargs...))


Test patch:
diff --git a/test/composition/models/stacking.jl b/test/composition/models/stacking.jl
index f420b916..555ae056 100644
--- a/test/composition/models/stacking.jl
+++ b/test/composition/models/stacking.jl
@@ -149,15 +149,33 @@ end
 end
 
 @testset "Stack constructor valid argument checks" begin
-    # metalearner should have target_scitype:
-    # Union{AbstractArray{<:Continuous}, AbstractArray{<:Finite}}
-    @test_throws ArgumentError Stack(;metalearner=Standardizer(),
-                        constant=ConstantClassifier())
-
-    @test_throws ArgumentError Stack(;constant=KNNRegressor())
+    # metalearner should be `Deterministic` or `Probablisitic`:
+    @test_throws(
+        MLJBase.ERR_BAD_METALEARNER,
+        Stack(;metalearner=Standardizer(), constant=ConstantClassifier()),
+    )
+    # must specify a metalearner:
+    @test_throws(
+        MLJBase.ERR_NO_METALEARNER,
+        Stack(;constant=ConstantRegressor()),
+    )
+    # informative error for spelling mistakes (#796):
+    @test_throws(
+        MLJBase.err_expecting_model(CPU1(), spelling=true),
+        Stack(metalearner=ConstantRegressor(), knn=ConstantRegressor(), acclraton=CPU1()),
+    )
+    # informative error is type used in place of instance (in a base model):
+    @test_throws(
+        MLJBase.err_expecting_model(ConstantRegressor),
+        Stack(metalearner=ConstantRegressor(), knn=ConstantRegressor),
+    )
+    # informative error is type used in place of instance (in metalearner):
+    @test_throws(
+        MLJBase.err_expecting_model(ConstantRegressor),
+        Stack(metalearner=ConstantRegressor, knn=ConstantRegressor()),
+    )
 end
 
-
 @testset "Misc" begin
     # Test setproperty! behaviour
     models = (constant=DeterministicConstantRegressor(),
diff --git a/test/models.jl b/test/models.jl
new file mode 100644
index 00000000..32b9092c
--- /dev/null
+++ b/test/models.jl
@@ -0,0 +1,42 @@
+struct Potato <: Model end
+
+@testset "message_expecting_model" begin
+
+    m1 = MLJBase.message_expecting_model(123)
+    @test !isnothing(match(r"(Expected)", m1))
+    @test isnothing(match(r"(type)", m1))
+    @test isnothing(match(r"(mispelled)", m1))
+
+    m2 = MLJBase.message_expecting_model(Potato)
+    @test !isnothing(match(r"(Expected)", m2))
+    @test !isnothing(match(r"(type)", m2))
+    @test isnothing(match(r"(mispelled)", m2))
+
+    m3 = MLJBase.message_expecting_model(123, spelling=true)
+    @test !isnothing(match(r"(Expected)", m3))
+    @test isnothing(match(r"(type)", m3))
+    @test !isnothing(match(r"(misspelled)", m3))
+
+    m4 = MLJBase.message_expecting_model(Potato, spelling=true)
+    @test !isnothing(match(r"(Expected)", m4))
+    @test !isnothing(match(r"(type)", m4))
+    @test isnothing(match(r"(misspelled)", m4))
+
+end
+
+@testset "check_ismodel" begin
+    @test isnothing(MLJBase.check_ismodel(Potato()))
+    @test_throws(
+        MLJBase.err_expecting_model(123),
+        MLJBase.check_ismodel(123),
+    )
+    @test_throws(
+        MLJBase.err_expecting_model(123, spelling=true),
+        MLJBase.check_ismodel(123, spelling=true),
+    )
+    @test_throws(
+        MLJBase.err_expecting_model(Potato),
+        MLJBase.check_ismodel(Potato),
+    )
+end
+true
diff --git a/test/runtests.jl b/test/runtests.jl
index c9b10f43..8ff6e93e 100644
--- a/test/runtests.jl
+++ b/test/runtests.jl
@@ -45,6 +45,10 @@ end
     @test include("sources.jl")
 end
 
+@conditional_testset "models" begin
+    @test include("models.jl")
+end
+
 @conditional_testset "machines" begin
     @test include("machines.jl")
 end