Repository navigation
Conversion of Hogwild SGD to estimator - #1134
Conversation
| pipe.Append(new StochasticGradientDescentClassificationTrainer(Env, "Features", "Label")); | ||
| TestEstimatorCore(pipe, dataView); | ||
| Done(); | ||
| } |
There was a problem hiding this comment.
nit, if there is only one test, you can also keep it on the TrainerEstimators file, to have a smaller amount of files. #Resolved
| [Argument(ArgumentType.AtMostOnce, HelpText = "L2 Regularization constant", ShortName = "l2", SortOrder = 50)] | ||
| [TGUI(Label = "L2 Regularization Constant", SuggestedSweeps = "1e-7,5e-7,1e-6,5e-6,1e-5")] | ||
| [TlcModule.SweepableDiscreteParam("L2Const", new object[] { 1e-7f, 5e-7f, 1e-6f, 5e-6f, 1e-5f })] | ||
| public float L2Const = (float)1e-6; |
There was a problem hiding this comment.
L2Const [](start = 25, length = 7)
double -check with @TomFinley on this change. Think we tend to keep the names same as the names of the variables in the respective papers. #Resolved
There was a problem hiding this comment.
There was a problem hiding this comment.
Checked with Tom and he is fine with the name change, as it seems commonly used in the rest of the codebase.
In reply to: 222463578 [](ancestors = 222463578,222460420)
| _args.L2Weight = l2Weight; | ||
| _args.Check(env); | ||
|
|
||
| _loss = _args.LossFunction.CreateComponent(env); |
There was a problem hiding this comment.
_loss = _a [](start = 10, length = 12)
i think Pete exposed the loss function for SDCA. Might want to do the same. #Resolved
| _args.WeightColumn = weightColumn; | ||
| _args.MaxIterations = maxIterations; | ||
| _args.InitLearningRate = initLearningRate; | ||
| _args.L2Weight = l2Weight; |
There was a problem hiding this comment.
for fast trees, i am printing a warning to the user to let them know that the values of the arguments are being overridden. Since we give them two places to set them, to avoid, feel like we should warn.
[https://github.com/dotnet/machinelearning/blob/master/src/Microsoft.ML.FastTree/FastTree.cs#L169](https://github.com/dotnet/machinelearning/blob/master/src/Microsoft.ML.FastTree/FastTree.cs#L169)
Also:
https://github.com/dotnet/machinelearning/blob/master/src/Microsoft.ML.FastTree/FastTree.cs#L104
#Resolved
| /// be informed about what was learnt.</param> | ||
| /// <returns>The predicted output.</returns> | ||
| public static (Scalar<float> score, Scalar<float> probability, Scalar<bool> predictedLabel) StochasticGradientDescentClassificationTrainer(this BinaryClassificationContext.BinaryClassificationTrainers ctx, | ||
| Scalar<bool> label, |
There was a problem hiding this comment.
Scalar label, [](start = 12, length = 19)
Can you ask Pete or Tom about this one?
I'm just concern what we have Pisgty only on top of bool label. In other universe we used to support R4 where positive label was everything above zero, and negative is everything else and Key value with restriction to key dimensionality is 2.
Bool value is fine, but what if user have Text label, like "Yes" and "No", I don't think we have currently way to transform them to bool, only ToKey, but this method limit us only to bool. #Closed
There was a problem hiding this comment.
There was a problem hiding this comment.
Ok I will check with them, and let you know.
In reply to: 222534836 [](ancestors = 222534836,222534640)
There was a problem hiding this comment.
I checked with Tom and he said that in Pigsty it is important to be very deliberate about the types, and that we will just allow the labels to be Scalar. This is a constraint of the static api.
In reply to: 222775240 [](ancestors = 222775240,222534836,222534640)
| /// <param name="features">The name of the feature column.</param> | ||
| /// <param name="weights">The name for the example weight column.</param> | ||
| /// <param name="maxIterations">The maximum number of iterations; set to 1 to simulate online learning.</param> | ||
| /// <param name="initLearningRate">The initial Learning Rate used by SGD.</param> |
There was a problem hiding this comment.
Learning R [](start = 55, length = 10)
nit: no need for title case #Resolved
There was a problem hiding this comment.
ghost
locked as resolved and limited conversation to collaborators
Ongoing work on converting the trainers to estimators (#754). This PR converts Hogwild SGD (StochasticGradientDescentClassificationTrainer binary classification trainer).