-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Throw exception in ImageClassificationTrainer when dataset contains only 1 class #4662
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Throw exception in ImageClassificationTrainer when dataset contains only 1 class #4662
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4662 +/- ##
=========================================
Coverage ? 75.83%
=========================================
Files ? 951
Lines ? 172306
Branches ? 18588
=========================================
Hits ? 130664
Misses ? 36466
Partials ? 5176
|
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.
🕐
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.
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.
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.
See my reply to the discussion on message clarity and consistency
@@ -603,7 +603,10 @@ private void InitializeTrainingGraph(IDataView input) | |||
(string)labelType.ToString()); | |||
} | |||
|
|||
_classCount = labelCount == 1 ? 2 : (int)labelCount; | |||
var msg = $"Only one class found in {_options.LabelColumnName} column. To build a multiclass classification model, the number of classes needs to be 2 or greater"; | |||
Contracts.CheckParam(labelCount > 1, nameof(labelCount), msg); |
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.
I've implemented the message that @natke suggested.
I had to use the "var msg" to store the string, because I couldn't use string interpolation inside the CheckParam() argument, because of this:
machinelearning/tools-local/Microsoft.ML.InternalCodeAnalyzer/Descriptions.Designer.cs
Lines 64 to 71 in fa75f4f
/// <summary> | |
/// Looks up a localized string similar to Since C# has no concept of lazy evaluation of parameters, we prefer Contracts.Check's message arguments to not involve string formatting, or other complex operations, since such operations will happen always, whether the check fails or not. If you want to have detailed messages that's great, but use Contracts.Except instead. That is instead of something like 'Check(c, msg)', prefer 'if (!c) throw Except(msg)'.. | |
/// </summary> | |
internal static string ContractsCheckMessageNotLiteralOrIdentifier { | |
get { | |
return ResourceManager.GetString("ContractsCheckMessageNotLiteralOrIdentifier", resourceCulture); | |
} | |
} |
So the suggestion there is to use Contracts.Except() instead of Contracts.Param() if I want to use string interpolation. Problem is Contracts.Except() throws an InvalidOperationException
(which @justinormont and @harishsk had suggested I shouldn't use). So I guess it's best to simply create the "msg" string before passing it to the Contracts.CheckParam()...
Throw exception in ImageClassificationTrainer when dataset contains only 1 class
Fixes #4660