Conversion of Parallel Stochastic Gradient Descent (SymSGD) to estimator #1012

artidoro · 2018-09-24T23:53:50Z

Ongoing work on converting the trainers to estimators. This PR converts the Parallel Stochastic Gradient Descent classification trainer (SymSGD).

…still need to pass the weight column

sfilipi · 2018-09-25T04:36:29Z

src/Microsoft.ML.HalLearners/SymSgdClassificationTrainer.cs

-    public sealed class SymSgdClassificationTrainer :
-        TrainerBase<TPredictor>,
-        ITrainer<TPredictor>
+    public sealed class SymSgdClassificationTrainer : TrainerEstimatorBase<BinaryPredictionTransformer<TPredictor>, TPredictor>


TrainerEstimatorBase [](start = 54, length = 20)

SingleFeatureTrainerEstimatorBase #Closed

Sorry I can't find that. what is the name of the base class I
should derive from?

In reply to: 220056284 [](ancestors = 220056284)

Why would this not work by the way?

In reply to: 220256710 [](ancestors = 220256710,220056284)

My bad, this is fine. got it confused with the TransformerPredictors

In reply to: 220256891 [](ancestors = 220256891,220256710,220056284)

sfilipi · 2018-09-25T04:36:52Z

src/Microsoft.ML.HalLearners/SymSgdClassificationTrainer.cs

-    public sealed class SymSgdClassificationTrainer :
-        TrainerBase<TPredictor>,
-        ITrainer<TPredictor>
+    public sealed class SymSgdClassificationTrainer : TrainerEstimatorBase<BinaryPredictionTransformer<TPredictor>, TPredictor>
    {
        public const string LoadNameValue = "SymbolicSGD";
        public const string UserNameValue = "Symbolic SGD (binary)";


public [](start = 8, length = 6)

internal, all 4 if you can. #Closed

sfilipi · 2018-09-25T04:39:37Z

        public float L2Regularization;

Might need to have a value for this on the new constructor; since no default. #Closed

Refers to: src/Microsoft.ML.HalLearners/SymSgdClassificationTrainer.cs:63 in 6697257. [](commit_id = 6697257, deletion_comment = False)

sfilipi · 2018-09-25T04:41:57Z

src/Microsoft.ML.HalLearners/SymSgdClassificationTrainer.cs

+            : base(Contracts.CheckRef(env, nameof(env)).Register(LoadNameValue), TrainerUtils.MakeR4VecFeature(featureColumn),
+                  TrainerUtils.MakeR4ScalarLabel(labelColumn), TrainerUtils.MakeR4ScalarWeightColumn(weightColumn))
+        {
+            var args = new Arguments();


var args [](start = 12, length = 8)

you can use _args directly. #Closed

sfilipi · 2018-09-25T04:43:25Z

src/Microsoft.ML.HalLearners/SymSgdClassificationTrainer.cs

+        /// </summary>
+        internal SymSgdClassificationTrainer(IHostEnvironment env, Arguments args)
+            : base(Contracts.CheckRef(env, nameof(env)).Register(LoadNameValue), TrainerUtils.MakeR4VecFeature(args.FeatureColumn),
+                  TrainerUtils.MakeR4ScalarLabel(args.LabelColumn), TrainerUtils.MakeR4ScalarWeightColumn(null))


MakeR4ScalarLabel [](start = 31, length = 17)

bool #Closed

You need a scalar bool label for binary classification.

In reply to: 220057042 [](ancestors = 220057042)

Thanks for the catch, I had only modified it above and forgot this instance.

In reply to: 220299186 [](ancestors = 220299186,220057042)

sfilipi · 2018-09-25T04:44:55Z

test/Microsoft.ML.Tests/TrainerEstimators/SymSgdClassificationTests.cs

+                    new TextLoader.Column("Features", DataKind.R4, new [] { new TextLoader.Range(1, 9) } )
+                }
+            }).Read(new MultiFileSource(GetDataPath(TestDatasets.breastCancer.trainFilename)));
+        }


There is a GetBinaryPipeline() methof on TrainerEstimators. See if it helps reduce duplication. #Closed

sfilipi · 2018-09-25T18:15:07Z

src/Microsoft.ML.HalLearners/SymSgdClassificationTrainer.cs

+            // Apply the advanced args, if the user supplied any.
+            _args.Check(Host);
+            advancedSettings?.Invoke(_args);
+            _args.FeatureColumn = featureColumn;


args.FeatureColumn [](start = 13, length = 18)

weights column #Resolved

Still missing weightColumn a couple commits after @sfilipi's comment

In reply to: 220300485 [](ancestors = 220300485)

…to symbolicsgd

TomFinley · 2018-09-25T19:59:34Z

test/Microsoft.ML.Tests/TrainerEstimators/SymSgdClassificationTests.cs

+        [Fact]
+        public void TestEstimatorSymSgdClassificationTrainer()
+        {
+            (IEstimator<ITransformer> pipe, IDataView dataView) = GetBinaryClassificationPipeline();


IEstimator [](start = 13, length = 24)

For things like this don't be afraid of var. #Closed

TomFinley · 2018-09-25T20:20:24Z

src/Microsoft.ML.HalLearners/SymSgdClassificationTrainer.cs

+        /// <param name="env">The private instance of <see cref="IHostEnvironment"/>.</param>
+        /// <param name="labelColumn">The name of the label column.</param>
+        /// <param name="featureColumn">The name of the feature column.</param>
+        /// <param name="weightColumn">The name for the column containing the initial weight.</param>


param name="weightColumn">The name for the column containing the initial weight. [](start = 13, length = 88)

If we don't support weighted training this should be removed, at least in its description of a column.

I think what was actually intended, because it says "initial weight," was that when you train, an initial predictor from which you get initial weights should be supported. (e.g., as we see here:)

machinelearning/src/Microsoft.ML.HalLearners/SymSgdClassificationTrainer.cs

Line 145 in 893a385

var initPred = context.InitialPredictor;

However this would be a parameter to the training/fitting method, not the constructor.

That is, I think the request was to have something akin to this auxiliary method here:

machinelearning/src/Microsoft.ML.StandardLearners/Standard/LinearClassificationTrainer.cs

Line 1495 in 893a385

public BinaryPredictionTransformer<TScalarPredictor> Train(IDataView trainData, IDataView validationData = null, IPredictor initialPredictor = null) => TrainTransformer(trainData, validationData, initialPredictor);

In reply to: 220340515 [](ancestors = 220340515)

That is actually already taken care of.
TrainerEstimatorBase has TrainTransformer method, which takes an initial predictor, and passes that to TrainModelCore through the TrainContext.

In reply to: 220342250 [](ancestors = 220342250,220340515)

You're talking about this I guess.

machinelearning/src/Microsoft.ML.Data/Training/TrainerEstimatorBase.cs

Lines 127 to 128 in d7b062d

protected TTransformer TrainTransformer(IDataView trainSet,

IDataView validationSet = null, IPredictor initPredictor = null)

@artidoro, no, as you see this method is protected. It is not accessible to people that instantiate the class. So, no, not taken care of.

Your PR was already merged, so whoops, too late I guess. Maybe we can file an issue to fix this post 0.6.

In reply to: 220345387 [](ancestors = 220345387,220342250,220340515)

…to symbolicsgd

Zruty0

artidoro · 2018-09-25T21:09:56Z

        public float L2Regularization;

After discussing with Pete, we should keep this as an advanced setting. The default value is 0 right now.

In reply to: 424204987 [](ancestors = 424204987)

Refers to: src/Microsoft.ML.HalLearners/SymSgdClassificationTrainer.cs:63 in 6697257. [](commit_id = 6697257, deletion_comment = False)

sfilipi

artidoro added 2 commits September 24, 2018 13:32

started conversion

5e8fcdb

conversion of symsgdclassificationtrainer, added new estimator test, …

6697257

…still need to pass the weight column

artidoro added the API Issues pertaining the friendly API label Sep 24, 2018

Zruty0 mentioned this pull request Sep 24, 2018

New API for ML.NET #754

Closed

sfilipi reviewed Sep 25, 2018

View reviewed changes

sfilipi self-requested a review September 25, 2018 04:45

fixed review comments

c12cd18

artidoro changed the title ~~WIP: Conversion of Parallel Stochastic Gradient Descent (SymSGD) to estimator~~ Conversion of Parallel Stochastic Gradient Descent (SymSGD) to estimator Sep 25, 2018

sfilipi reviewed Sep 25, 2018

View reviewed changes

artidoro added 2 commits September 25, 2018 11:25

Merge branch 'master' of https://github.com/dotnet/machinelearning in…

13aef31

…to symbolicsgd

fixed review comment

f7f6666

TomFinley reviewed Sep 25, 2018

View reviewed changes

artidoro added 2 commits September 25, 2018 13:37

Merge branch 'master' of https://github.com/dotnet/machinelearning in…

f602a04

…to symbolicsgd

fixed more review comments

2d2faa9

Zruty0 approved these changes Sep 25, 2018

View reviewed changes

added l2 regularization coefficient as a parameter to the constructor

7877fe0

reverting the l2 regularization change

79d9066

sfilipi approved these changes Sep 25, 2018

View reviewed changes

shauheen added this to the 0918 milestone Sep 25, 2018

shauheen merged commit ab32439 into dotnet:master Sep 25, 2018

ghost locked as resolved and limited conversation to collaborators Mar 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Conversion of Parallel Stochastic Gradient Descent (SymSGD) to estimator #1012

Conversion of Parallel Stochastic Gradient Descent (SymSGD) to estimator #1012

artidoro commented Sep 24, 2018

sfilipi Sep 25, 2018 •

edited

Loading

artidoro Sep 25, 2018

artidoro Sep 25, 2018

sfilipi Sep 25, 2018

sfilipi Sep 25, 2018 •

edited

Loading

sfilipi commented Sep 25, 2018 •

edited

Loading

sfilipi Sep 25, 2018 •

edited

Loading

sfilipi Sep 25, 2018 •

edited

Loading

sfilipi Sep 25, 2018

artidoro Sep 25, 2018

sfilipi Sep 25, 2018 •

edited

Loading

sfilipi Sep 25, 2018 •

edited by artidoro

Loading

TomFinley Sep 25, 2018

TomFinley Sep 25, 2018 •

edited

Loading

TomFinley Sep 25, 2018 •

edited

Loading

TomFinley Sep 25, 2018

artidoro Sep 25, 2018 •

edited

Loading

TomFinley Sep 26, 2018

Zruty0 left a comment

artidoro commented Sep 25, 2018

sfilipi left a comment

	protected TTransformer TrainTransformer(IDataView trainSet,
	IDataView validationSet = null, IPredictor initPredictor = null)

Conversion of Parallel Stochastic Gradient Descent (SymSGD) to estimator #1012

Conversion of Parallel Stochastic Gradient Descent (SymSGD) to estimator #1012

Conversation

artidoro commented Sep 24, 2018

sfilipi Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

sfilipi commented Sep 25, 2018 • edited Loading

sfilipi Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

sfilipi Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

sfilipi Sep 25, 2018 • edited by artidoro Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

TomFinley Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artidoro Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 left a comment

Choose a reason for hiding this comment

artidoro commented Sep 25, 2018

sfilipi left a comment

Choose a reason for hiding this comment

sfilipi Sep 25, 2018 •

edited

Loading

sfilipi Sep 25, 2018 •

edited

Loading

sfilipi commented Sep 25, 2018 •

edited

Loading

sfilipi Sep 25, 2018 •

edited

Loading

sfilipi Sep 25, 2018 •

edited

Loading

sfilipi Sep 25, 2018 •

edited

Loading

sfilipi Sep 25, 2018 •

edited by artidoro

Loading

TomFinley Sep 25, 2018 •

edited

Loading

TomFinley Sep 25, 2018 •

edited

Loading

artidoro Sep 25, 2018 •

edited

Loading