Clarify error for uninstantiated model further

[rikhuijzer]

I was trying to do some hyperparameter tuning via

tunedmodel = let
    model = LogisticClassifier
    tuning = Grid()
    range = [
        range(Float64, :lambda; lower=0.01, upper=1, scale=:log),
        range(Float64, :gamma; lower=0.01, upper=1, scale=:log)
    ]
    measure = auc
    TunedModel(; model, tuning, range, measure)
end

and got

ArgumentError: Only `Deterministic` and `Probabilistic` model types supported.

which is very confusing. I had no idea what was the problem here. Fun fact, I got mad at the person who came up with this unclear error message. Turns out it was me https://github.com/JuliaAI/MLJTuning.jl/pull/157 :rofl:

Anyway, this PR allows passing model types so that the code above will "automatically" work. I considered throwing a more clear error, but accepting a model type makes more sense IMO since the tuning strategy will instantiate many models.

Two thoughts:

• Maybe the instantiate which I wrapped in a try-catch should take the first elements for the range for the first instantiation. Or isn't the instantiated model used anyway?
• The TunedModel should probably be refactored since it is getting more and more tough to reason about it. We can either do a breaking change and explicitly create TunedModel, TunedModels and other constructors so that we avoid logic like "if explicit is set and models is not set, then do .... Or, we could first figure out what kind of thing the user would like to do and dispatch into the appropriate function at the start of the TunedModels constructor

[ablaom]

@rikhuijzer Thanks for this.

I'm not convinced the inconvenience of typing two brackets warrants the introduction of a try-catch block. My vote would be to go with a better error message. How about this one:

https://github.com/JuliaAI/MLJBase.jl/blob/2257471e854a24eba0de7b71e22c8dcb2d3ec9b4/src/machines.jl#L143

@OkonSamuel What do you think?

The TunedModel should probably be refactored since it is getting more and more tough to reason about it.

I don't mind a clean up of the implementation. But you seem to be saying you find it confusing for the user also? Could you say more about that?

I'm not too enthusiastic about splitting this into multiple methods as there are already so many to remember. Perhaps you have a different suggestion?

[OkonSamuel]

@OkonSamuel What do you think?

Am not really a fan of using try-catch blocks in code. Except as a last resort. Maybe the refactoring of TunedModels is a better option.

[rikhuijzer]

I'm not convinced the inconvenience of typing two brackets warrants the introduction of a try-catch block.

I don't mind a clean up of the implementation. But you seem to be saying you find it confusing for the user also? Could you say more about that?

For me it was confusing that the tuning takes some model and it's parameters, so I assume then that the tuning constructs all my models for me. As an user, it isn't clear why there is a need to manually instantiate the model. Even more, I would normally assume that I should only pass a model type.

[ablaom]

For me it was confusing that the tuning takes some model and it's parameters, so I assume then that the tuning constructs all my models for me. As an user, it isn't clear why there is a need to manually instantiate the model. Even more, I would normally assume that I should only pass a model type.

Thanks for pointing out your confusion.

Yes, tuning for non-Explicit strategies constructs of all the models by cloning the provided model and mutating just those hyper parameters specified by the range. So what particular instance you provide makes a difference. I think this is clear from here but I can see this is not spelled out in the MLJ manual. So, happy to have that clarified.

In the case of specifying an explicit list of models, there is no mutation of a fixed model and obviously which instances you provide makes a difference.

Is that clearer?

[rikhuijzer]

Yes, tuning for non-Explicit strategies constructs of all the models by cloning the provided model and mutating just those hyper parameters specified by the range. So what particular instance you provide makes a difference. I think this is clear from here but I can see this is not spelled out in the MLJ manual. So, happy to have that clarified.

Now it all makes sense! :smile:

How about this (f699f67)?

julia> using MLJBase, MLJTuning

julia model = DeterministicSupervisedDetector;

julia> TunedModel(; model, range=[1])
ERROR: AssertionError: Uninstantiated DeterministicSupervisedDetector passed; pass an instantiated model so that this package can mutate the hyperparameters specified by the range.

In a perfect world, users would have read the manual indeed @ablaom :smile: Unfortunately, probably not everyone does. For those people, let's figure out their mistake and give them a clear solution.

[ablaom]

In a perfect world, users would have read the manual indeed @ablaom 😄 Unfortunately, probably not everyone does. For those people, let's figure out their mistake and give them a clear solution.

Agreed. A more informative error message very often beats doc-strings. Appreciate the feedback!