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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clarify error for uninstantiated model further #178
Conversation
@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: @OkonSamuel What do you think?
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? |
Am not really a fan of using try-catch blocks in code. Except as a last resort. Maybe the refactoring of |
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- 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? |
Now it all makes sense! 馃槃 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 馃槃 Unfortunately, probably not everyone does. For those people, let's figure out their mistake and give them a clear solution. |
model
Agreed. A more informative error message very often beats doc-strings. Appreciate the feedback! |
Co-authored-by: Anthony Blaom, PhD <anthony.blaom@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rikhuijzer for this contribution.
I was trying to do some hyperparameter tuning via
and got
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 #157 馃ぃ
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:
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 createTunedModel
,TunedModels
and other constructors so that we avoid logic like "ifexplicit
is set andmodels
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 theTunedModels
constructor