Modify API for advanced settings (several learners) #2163

abgoswam · 2019-01-16T20:24:17Z

Towards #1798 .

This PR addresses the following algos

LogisticRegression
MulticlassLogisticRegression
PoissonRegression
AveragedPerceptronTrainer
LinearSvmTrainer
OnlineGradientDescentTrainer

The following changes have been made:

Two public extension methods, one for simple arguments and the other for advanced options
Make constructors internal
Pass Options objects as arguments instead of Action delegate
Rename Arguments to Options
Rename Options objects as options (instead of args or advancedSettings used so far)

srsaggam · 2019-01-16T23:43:28Z

@abgoswam What if user wants to set certain advance settings for the trainer. How is he going to do it now? Previously, AdvanceSettings Func was the way to do it. #Resolved

srsaggam · 2019-01-16T23:46:50Z

My Bad. you have renamed it to Options. Nevermind. Thanks #Resolved

sfilipi · 2019-01-17T05:28:10Z

src/Microsoft.ML.Ensemble/Trainer/Binary/EnsembleTrainer.cs

-                        env => new LinearSvmTrainer(env))
+                        env => {
+                            var mlContext = new MLContext();
+                            return mlContext.BinaryClassification.Trainers.LinearSupportVectorMachines();


i don't think it is a good idea to create another context/environment.
Just use the constructor, for internal code.

Add the [BestFriend] annotation to LinearSVM, if it canot be accessed from here. #Closed

thanks. for the tip about using the [BestFriend] attribute

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

sfilipi · 2019-01-17T05:28:58Z

src/Microsoft.ML.Ensemble/Trainer/Multiclass/MulticlassDataPartitionEnsembleTrainer.cs

-                        env => new MulticlassLogisticRegression(env, LabelColumn, FeatureColumn))
+                        env => {
+                            var mlContext = new MLContext();
+                            return mlContext.MulticlassClassification.Trainers.LogisticRegression(LabelColumn, FeatureColumn);


here too. #Resolved

fixed

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

sfilipi · 2019-01-17T05:29:11Z

src/Microsoft.ML.Ensemble/Trainer/Regression/RegressionEnsembleTrainer.cs

+                        env => {
+                            var mlContext = new MLContext();
+                            return mlContext.Regression.Trainers.OnlineGradientDescent();
+                        })


one more #Resolved

fixed

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

sfilipi · 2019-01-17T05:30:26Z

src/Microsoft.ML.StandardLearners/StandardLearnersCatalog.cs

+        }
+
+        /// <summary>
+        /// Predict a target using a linear binary classification model trained with the AveragedPerceptron trainer, and a custom loss.


r, and a custom loss [](start = 114, length = 20)

maybe doesn't make sense to call out the loss here. #Closed

agree. also removed it from the non-Options one

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

sfilipi · 2019-01-17T05:30:52Z

src/Microsoft.ML.StandardLearners/StandardLearnersCatalog.cs

+        }
+
+        /// <summary>
+        /// Predict a target using a linear binary classification model trained with the AveragedPerceptron trainer, and a custom loss.


AveragedPerceptron [](start = 89, length = 18)

on type references.

I don't think we need to add a type reference here. Do we ?

My sense is we are using it to refer to the Averaged Perceptron algo, and not to a specific type within ML.NET

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

it says AveragePerception trainer. I think it would be good to point to the actual trainer class, so they can read more about the algo. Leaving it like this will cause the users to.. web search.

In reply to: 248786650 [](ancestors = 248786650,248543067)

sfilipi · 2019-01-17T05:31:11Z

src/Microsoft.ML.StandardLearners/StandardLearnersCatalog.cs

+        /// Predict a target using a linear binary classification model trained with the AveragedPerceptron trainer, and a custom loss.
+        /// </summary>
+        /// <param name="ctx">The binary classification context trainer object.</param>
+        /// <param name="options">Advanced arguments to the algorithm.</param>


arguments [](start = 43, length = 9)

options, here and in the other ones.
I didn't do it in my PR, but thinking now that it might make sense to to the Options class, so users can see the options. #Resolved

Should we do it after all the code changes are in ? Perhaps as part of #2154 ?

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

You have it in your list of things to do :)
Up to you, as long as it gets done.

In reply to: 248787187 [](ancestors = 248787187,248543101)

I added it to description of #2154

In reply to: 248835228 [](ancestors = 248835228,248787187,248543101)

sfilipi · 2019-01-17T05:32:28Z

src/Microsoft.ML.StandardLearners/StandardLearnersCatalog.cs

+        /// Predict a target using a linear regression model trained with the <see cref="OnlineGradientDescentTrainer"/> trainer.
+        /// </summary>
+        /// <param name="ctx">The regression context trainer object.</param>
+        /// <param name="options">Advanced arguments to the algorithm.</param>


arguments [](start = 43, length = 9)

here #Resolved

sfilipi · 2019-01-17T05:32:58Z

src/Microsoft.ML.StandardLearners/StandardLearnersCatalog.cs

+        ///  Predict a target using a linear binary classification model trained with the <see cref="Microsoft.ML.Learners.LogisticRegression"/> trainer.
+        /// </summary>
+        /// <param name="ctx">The binary classificaiton context trainer object.</param>
+        /// <param name="options">Advanced arguments to the algorithm.</param>


arguments [](start = 43, length = 9)

options #Resolved

sfilipi · 2019-01-17T05:35:06Z

src/Microsoft.ML.StandardLearners/StandardLearnersCatalog.cs

        {
            Contracts.CheckValue(ctx, nameof(ctx));
            var env = CatalogUtils.GetEnvironment(ctx);
-            return new PoissonRegression(env, labelColumn, featureColumn, weights, l1Weight, l2Weight, optimizationTolerance, memorySize, enforceNoNegativity, advancedSettings);
+            return new PoissonRegression(env, labelColumn, featureColumn, weights, l1Weight, l2Weight, optimizationTolerance, memorySize, enforceNoNegativity);


PoissonRegression [](start = 23, length = 17)

does it make sense to call the other ctor, the new PoissonRegression(env, options); and just delete this ctor on this PR, or you'll tackle it later? #Resolved

For unification of internal constructors I created a separate issue #2100 , which we will tackle later

The priority for now is to get the public API (i.e. MLContext) straightened out

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

sfilipi · 2019-01-17T05:36:31Z

src/Microsoft.ML.StandardLearners/Standard/LogisticRegression/LogisticRegression.cs

@@ -74,18 +74,16 @@ public sealed class Arguments : ArgumentsBase
        /// <param name="l2Weight">Weight of L2 regularizer term.</param>
        /// <param name="memorySize">Memory size for <see cref="LogisticRegression"/>. Low=faster, less accurate.</param>
        /// <param name="optimizationTolerance">Threshold for optimizer convergence.</param>
-        /// <param name="advancedSettings">A delegate to apply all the advanced arguments to the algorithm.</param>
-        public LogisticRegression(IHostEnvironment env,
+        internal LogisticRegression(IHostEnvironment env,
            string labelColumn = DefaultColumnNames.Label,
            string featureColumn = DefaultColumnNames.Features,
            string weights = null,


i'd just delete it.. #Resolved

I do see it being passed to base constructor LbfgsTrainerBase where its used to initialize WeightsColumn Thoughts ?

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

The ctor. Sorry for the bad selection.

In reply to: 248769580 [](ancestors = 248769580,248543741)

Deleting the constructor u mean ? That would be #2100

In reply to: 248834382 [](ancestors = 248834382,248769580,248543741)

sfilipi · 2019-01-17T05:37:19Z

src/Microsoft.ML.StandardLearners/Standard/LogisticRegression/MulticlassLogisticRegression.cs

        MulticlassPredictionTransformer<MulticlassLogisticRegressionModelParameters>, MulticlassLogisticRegressionModelParameters>
    {
        public const string LoadNameValue = "MultiClassLogisticRegression";
        internal const string UserNameValue = "Multi-class Logistic Regression";
        internal const string ShortName = "mlr";

-        public sealed class Arguments : ArgumentsBase
+        public sealed class Options : ArgumentsBase
        {
            [Argument(ArgumentType.AtMostOnce, HelpText = "Show statistics of training examples.", ShortName = "stat", SortOrder = 50)]
            public bool ShowTrainingStats = false;


ShowTrainingStats [](start = 24, length = 17)

xml #Resolved

for xml updates, i created #2154 to fix these across the board once code changes is complete

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

sfilipi · 2019-01-17T05:38:05Z

src/Microsoft.ML.StandardLearners/Standard/Online/AveragedPerceptron.cs


-        public sealed class Arguments : AveragedLinearArguments
+        public sealed class Options : AveragedLinearArguments
        {
            [Argument(ArgumentType.Multiple, HelpText = "Loss Function", ShortName = "loss", SortOrder = 50)]
            public ISupportClassificationLossFactory LossFunction = new HingeLoss.Arguments();


ISupportClassificationLossFactory [](start = 19, length = 33)

xml #Resolved

for xml summary updates, i created #2154 to fix these across the board once code changes have been checked-in

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

sfilipi · 2019-01-17T05:38:37Z

src/Microsoft.ML.StandardLearners/Standard/Online/AveragedPerceptron.cs

@@ -103,18 +103,16 @@ internal AveragedPerceptronTrainer(IHostEnvironment env, Arguments args)
        /// <param name="decreaseLearningRate">Wheather to decrease learning rate as iterations progress.</param>
        /// <param name="l2RegularizerWeight">L2 Regularization Weight.</param>
        /// <param name="numIterations">The number of training iteraitons.</param>
-        /// <param name="advancedSettings">A delegate to supply more advanced arguments to the algorithm.</param>
-        public AveragedPerceptronTrainer(IHostEnvironment env,
+        internal AveragedPerceptronTrainer(IHostEnvironment env,
            string labelColumn = DefaultColumnNames.Label,
            string featureColumn = DefaultColumnNames.Features,
            string weights = null,


delete.. #Resolved

I do see it being used to initialize InitialWeights. Thoughts ?

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

I meant the constructor, bad selection.

In reply to: 248768887 [](ancestors = 248768887,248544047)

Deleting the constructor u mean ? That would be #2100

In reply to: 248834137 [](ancestors = 248834137,248768887,248544047)

sfilipi · 2019-01-17T05:41:50Z

src/Microsoft.ML.StaticPipe/OnlineLearnerStatic.cs

+                {
+                    options.LabelColumn = labelName;
+                    options.FeatureColumn = featuresName;
+                    options.InitialWeights = weightsName;


InitialWeights [](start = 28, length = 14)

just checking that this is not an Optional #Closed

yeap . its of type string in OnlineLinear.cs
public string InitialWeights;

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

sfilipi · 2019-01-17T05:45:47Z

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

@@ -36,16 +36,14 @@ public void OnlineLinearWorkout()
                .Append(r => (r.Label, Features: r.Features.Normalize()));

            var binaryTrainData = binaryPipe.Fit(binaryData).Transform(binaryData).AsDynamic;
-            var apTrainer = new AveragedPerceptronTrainer(ML, "Label", "Features", lossFunction: new HingeLoss(), advancedSettings: s =>


lossFunction: new HingeLoss() [](start = 83, length = 29)

set this inside Options, so we keep the test the same. #Pending

Created issue #2174. We need to fix how LossFunction is defined for AvgPerceptron and OnlineGradientDescent

Unfortunately, i cant add a "skipped" test because its a compile time error (not a run-time issue).

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

sfilipi

abgoswam added 2 commits January 16, 2019 19:40

LogisticRegression, MulticlassLogisticRegression, PoissonRegression

961fe9d

Options rename

91939d9

abgoswam requested review from sfilipi and artidoro January 16, 2019 21:15

weights updates

fed254d

abgoswam added 4 commits January 17, 2019 00:55

AveragedPerceptron

c447d96

OnlineGradientDescent

7d15dfb

LinearSvm

5cf6c19

Options rename

c89741f

abgoswam changed the title ~~Modify API for advanced settings {LogisticRegression, MulticlassLogisticRegression, PoissonRegression}~~ Modify API for advanced settings (several learners) Jan 17, 2019

sfilipi reviewed Jan 17, 2019

View reviewed changes

sfilipi assigned abgoswam Jan 17, 2019

sfilipi added the API Issues pertaining the friendly API label Jan 17, 2019

review comments

5b316f2

sfilipi approved these changes Jan 17, 2019

View reviewed changes

artidoro approved these changes Jan 17, 2019

View reviewed changes

merge with latest master

ea9a5de

abgoswam merged commit bb92c06 into dotnet:master Jan 18, 2019

abgoswam deleted the abgoswam/action_arguments_4 branch January 21, 2019 21:25

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

Modify API for advanced settings (several learners) #2163

Modify API for advanced settings (several learners) #2163

Conversation

abgoswam commented Jan 16, 2019 • edited Loading

srsaggam commented Jan 16, 2019 • edited by abgoswam Loading

srsaggam commented Jan 16, 2019 • edited by abgoswam Loading

sfilipi Jan 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Jan 17, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Jan 17, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Jan 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Jan 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Jan 17, 2019 • edited by abgoswam 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 Jan 17, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

sfilipi Jan 17, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

sfilipi Jan 17, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Jan 17, 2019 • edited by abgoswam 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 Jan 17, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

abgoswam Jan 17, 2019 • edited Loading

Choose a reason for hiding this comment

sfilipi Jan 17, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Jan 17, 2019 • edited by abgoswam 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 Jan 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Jan 17, 2019 • edited by abgoswam Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi left a comment

Choose a reason for hiding this comment

abgoswam commented Jan 16, 2019 •

edited

Loading

srsaggam commented Jan 16, 2019 •

edited by abgoswam

Loading

srsaggam commented Jan 16, 2019 •

edited by abgoswam

Loading

sfilipi Jan 17, 2019 •

edited

Loading

sfilipi Jan 17, 2019 •

edited by abgoswam

Loading

sfilipi Jan 17, 2019 •

edited by abgoswam

Loading

sfilipi Jan 17, 2019 •

edited

Loading

sfilipi Jan 17, 2019 •

edited

Loading

sfilipi Jan 17, 2019 •

edited by abgoswam

Loading

sfilipi Jan 17, 2019 •

edited by abgoswam

Loading

sfilipi Jan 17, 2019 •

edited by abgoswam

Loading

sfilipi Jan 17, 2019 •

edited by abgoswam

Loading

sfilipi Jan 17, 2019 •

edited by abgoswam

Loading

sfilipi Jan 17, 2019 •

edited by abgoswam

Loading

abgoswam Jan 17, 2019 •

edited

Loading

sfilipi Jan 17, 2019 •

edited by abgoswam

Loading

sfilipi Jan 17, 2019 •

edited by abgoswam

Loading

sfilipi Jan 17, 2019 •

edited

Loading

sfilipi Jan 17, 2019 •

edited by abgoswam

Loading