-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Make Multiclass Linear Trainers Typed Based on Output Model Types. #2976
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2976 +/- ##
==========================================
+ Coverage 72.38% 72.4% +0.02%
==========================================
Files 803 803
Lines 143569 143851 +282
Branches 16162 16173 +11
==========================================
+ Hits 103924 104160 +236
- Misses 35227 35267 +40
- Partials 4418 4424 +6
|
// verWrittenCur: 0x00010001, // Initial | ||
// verWrittenCur: 0x00010002, // Added class names | ||
verWrittenCur: 0x00010003, // Added model stats | ||
verReadableCur: 0x00010001, |
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.
As far as I know this is a new model type. Why is this not the initial version? #Resolved
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.
@@ -171,7 +171,7 @@ private ITransformer TrainOnIris(string irisDataPath) | |||
var trainedModel = pipeline.Fit(trainData); | |||
|
|||
// Inspect the model parameters. | |||
var modelParameters = trainedModel.LastTransformer.Model as MulticlassLogisticRegressionModelParameters; | |||
var modelParameters = trainedModel.LastTransformer.Model as MaximumEntropyModelParameters; |
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.
MaximumEntropyModelParameters [](start = 72, length = 29)
obligatory "Update md file" comment #Closed
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.
/// <example> | ||
/// <format type="text/markdown"> | ||
/// <] |
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.
StochasticDualCoordinateAscent [](start = 127, length = 30)
Should you add new sample file for non calibrated version? #Pending
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 don't want to touch too many those APIs as they are being renamed and moved.
In reply to: 266641719 [](ancestors = 266641719)
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.
Then don't add example? I don't know how our documentation people track their work, but looks like it has everything it's needed, but in reality it's not.
In reply to: 266650832 [](ancestors = 266650832,266641719)
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.
This PR fixes a big wrong thing we have been doing for years (as you can see, we need so many changes to fix it). I personally consider example is rather less important. In addition, we now postpone all documentation tasks.
In reply to: 266987401 [](ancestors = 266987401,266650832,266641719)
{ | ||
internal const string Summary = "Logistic Regression is a method in statistics used to predict the probability of occurrence of an event and can be used as a classification algorithm.The algorithm predicts the probability of occurrence of an event by fitting data to a logistical function."; |
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.
space #Closed
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.
/// </summary> | ||
public IEnumerable<float> GetBiases() | ||
{ | ||
return Biases; | ||
} | ||
|
||
internal IEnumerable<float> DenseWeightsEnumerable() |
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.
Not exactly related to your PR, but I found it strange what we expose weights and Bias(es) differently for binary and multiclass model.
#ByDesign
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.
The reason behind is that those parameters got one extra dimension. In binary, the bias is a scalar but in multiclass, the bias is a vector with #-of-class elements.
In reply to: 266661678 [](ancestors = 266661678)
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.
Which is weird.
NumClasses should be property of the model, bias and weights should be IReadonly etc, but all this is out of scope of this PR:)
In reply to: 266668443 [](ancestors = 266668443,266661678)
private protected override void Calibrate(Span<float> dst) | ||
{ | ||
Host.Assert(dst.Length >= NumberOfClasses); | ||
|
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.
Why is dst.Lenght> NumberOfClasses
is ok? #Closed
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.
Step 2: Make SDCA trainers typed Finish version 0.1 Delete commented lines
More document
Fix two tests and address a comment Add missing piece
|
||
namespace Microsoft.ML.Trainers | ||
{ | ||
/// <include file = 'doc.xml' path='doc/members/member[@name="LBFGS"]/*' /> | ||
/// <include file = 'doc.xml' path='docs/members/example[@name="LogisticRegressionClassifier"]/*' /> | ||
public sealed class LogisticRegressionMulticlassClassificationTrainer : LbfgsTrainerBase<LogisticRegressionMulticlassClassificationTrainer.Options, | ||
MulticlassPredictionTransformer<MulticlassLogisticRegressionModelParameters>, MulticlassLogisticRegressionModelParameters> | ||
public sealed class LbfgsMaximumEntropyTrainer : LbfgsTrainerBase<LbfgsMaximumEntropyTrainer.Options, |
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.
Can we have prefix match between the Trainer and ModelParameter class . Perhaps call the trainer MaximumEntropyTrainer
? #Closed
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.
This is not the only trainer that will produce MaximumEntropyTrainer.. Please consider this trainer and the associated model separately. Otherwise, every trainer which produces linear model should be called LinearModelTrainer.
In reply to: 266683935 [](ancestors = 266683935)
/// <param name="optimizationTolerance">Threshold for optimizer convergence.</param> | ||
public static LogisticRegressionMulticlassClassificationTrainer LogisticRegression(this MulticlassClassificationCatalog.MulticlassClassificationTrainers catalog, | ||
public static LbfgsMaximumEntropyTrainer LbfgsMaximumEntropy(this MulticlassClassificationCatalog.MulticlassClassificationTrainers catalog, |
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.
Its great we are following convention of having prefix match with the name of Trainer #Resolved
/// <include file='doc.xml' path='doc/members/member[@name="SDCA_remarks"]/*' /> | ||
public sealed class SdcaNonCalibratedMulticlassClassificationTrainer : SdcaMulticlassClassificationTrainerBase<LinearMulticlassModelParameters> | ||
{ | ||
public class Options : CommonOptions |
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.
public class Options [](start = 8, length = 20)
sealed? #Closed
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.
/// <include file='doc.xml' path='doc/members/member[@name="SDCA_remarks"]/*' /> | ||
public sealed class SdcaMulticlassClassificationTrainer : SdcaMulticlassClassificationTrainerBase<MaximumEntropyModelParameters> | ||
{ | ||
public class Options : CommonOptions |
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.
public class Options [](start = 8, length = 21)
sealed? #Closed
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.
() => TrainerEntryPointsUtils.FindColumn(host, input.TrainingData.Schema, input.LabelColumnName), | ||
() => TrainerEntryPointsUtils.FindColumn(host, input.TrainingData.Schema, input.ExampleWeightColumnName)); | ||
/// <summary> | ||
/// Output the text model to a given writer |
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.
Output the text model to a given writer [](start = 11, length = 40)
nit: dot in the end of sentence for all this comments in file. #Resolved
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. I just checked both of this file and SdcaMulticlass.cs.
In reply to: 266993940 [](ancestors = 266993940)
|
||
// Step 4: Make prediction and evaluate its quality (on training set). | ||
var prediction = model.Transform(data); | ||
var metrics = mlContext.MulticlassClassification.Evaluate(prediction, label: "LabelIndex", topK: 1); |
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.
label [](start = 82, length = 5)
I think @artidoro rename them to labelColumnName in his latest PR. #Closed
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.
/// [!code-csharp[SDCA](~/../docs/samples/docs/samples/Microsoft.ML.Samples/Dynamic/Trainers/MulticlassClassification/StochasticDualCoordinateAscentWithOptions.cs)] | ||
/// ]]></format> | ||
/// </example> | ||
public static SdcaCalibratedMulticlassTrainer SdcaCalibrated(this MulticlassClassificationCatalog.MulticlassClassificationTrainers catalog, |
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.
SdcaCalibratedMulticlassTrainer [](start = 22, length = 31)
nit: if you are doing another iteration, would be great if you keep the two extensions for SdcaCalibratedMulticlassTraniner
one after the other. #Resolved
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.
Can we drop the Refers to: src/Microsoft.ML.StandardTrainers/StandardTrainersCatalog.cs:519 in 63b46cb. [](commit_id = 63b46cb, deletion_comment = False) |
@@ -537,7 +587,7 @@ public static PoissonRegressionTrainer PoissonRegression(this RegressionCatalog. | |||
} | |||
|
|||
/// <summary> | |||
/// Predict a target using a linear multiclass classification model trained with the <see cref="LogisticRegressionMulticlassClassificationTrainer"/> trainer. | |||
/// Predict a target using a linear multiclass classification model trained with the <see cref="LbfgsMaximumEntropyTrainer"/> trainer. | |||
/// </summary> |
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.
Do you think it's worth it to add a few words saying that this is the multi class extension of logistic regression? (here and below) #Resolved
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.
It will become
/// Predict a target using a maximum entropy classification model trained with the L-BFGS method implemented in <see cref="LbfgsMaximumEntropyTrainer"/>.
In reply to: 267062024 [](ancestors = 267062024)
writer.WriteLine("output[{0}] = Math.Exp(scores[{0}] - softmax);", c); | ||
} | ||
|
||
private protected override bool SaveAsOnnxCore(OnnxContext ctx, string[] outputs, string featureColumn) |
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.
SaveAsOnnxCore [](start = 40, length = 14)
It looks like the following three methods are very similar, if not the same, for LinearMulticlassModelParameters
and MaximumEntropyModelParameters
.
Can they be refactored in the base class? #Resolved
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 need to do it in another PR if you really want (#3016). It will cause another batch of conflicts and is not related to this issue. In reply to: 474542216 [](ancestors = 474542216) Refers to: src/Microsoft.ML.StandardTrainers/StandardTrainersCatalog.cs:519 in 63b46cb. [](commit_id = 63b46cb, deletion_comment = False) |
Ok, sounds good! In reply to: 474562462 [](ancestors = 474562462,474542216) Refers to: src/Microsoft.ML.StandardTrainers/StandardTrainersCatalog.cs:519 in 63b46cb. [](commit_id = 63b46cb, deletion_comment = False) |
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.
Multiclass SDCA can train multi-class SVM but it always outputs multi-class logistic regression model. This is not correct because we should not apply softmax to SVM model.
To fix #1100, we got the following working items.
Framework changes:
API changes: