Towards #1798 . #2170

sfilipi · 2019-01-17T06:25:53Z

This PR addresses the estimators inside HalLearners:

Two public extension methods, one for simple arguments and the other for advanced options
Delete unecessary constructors
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)

rogancarr

Approved with some questions / comments!

rogancarr · 2019-01-17T17:26:53Z

src/Microsoft.ML.HalLearners/HalLearnersCatalog.cs

+            string weights = null)
+        {
+            Contracts.CheckValue(ctx, nameof(ctx));
+            var env = CatalogUtils.GetEnvironment(ctx);


Why do we do this when the other API needs the same context? #Resolved

the new convention is to have two APIs: one for basic usage that let's you set the column names, and most used params, the other one that takes in Options, and allows to set everything.

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

Oh, I meant, why do we generate a new env when the other API needs the same context as we have in ctx? #Resolved

the trainer ctor needs an IHostEnvironment.
The
CatalogUtils.GetEnvironment(ctx)

does not generate a new environment, just gets the one from within the context.

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

rogancarr · 2019-01-17T17:28:25Z

src/Microsoft.ML.HalLearners/HalLearnersCatalog.cs

            string labelColumn = DefaultColumnNames.Label,
            string featureColumn = DefaultColumnNames.Features,
-            Action<SymSgdClassificationTrainer.Arguments> advancedSettings = null)
+            Action<SymSgdClassificationTrainer.Options> advancedSettings = null)


This argument is unused. Drop it? #Resolved

thanks

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

rogancarr · 2019-01-17T17:29:06Z

src/Microsoft.ML.HalLearners/HalLearnersCatalog.cs

        /// <param name="labelColumn">The labelColumn column.</param>
        /// <param name="featureColumn">The features column.</param>
        /// <param name="advancedSettings">Algorithm advanced settings.</param>
        public static SymSgdClassificationTrainer SymbolicStochasticGradientDescent(this BinaryClassificationContext.BinaryClassificationTrainers ctx,


Too long? #Resolved

rogancarr · 2019-01-17T17:29:16Z

src/Microsoft.ML.HalLearners/HalLearnersCatalog.cs

+        /// </summary>
+        /// <param name="ctx">The <see cref="BinaryClassificationContext"/>.</param>
+        /// <param name="options">Algorithm advanced options.</param>
+        public static SymSgdClassificationTrainer SymbolicStochasticGradientDescent(this BinaryClassificationContext.BinaryClassificationTrainers ctx,


Too long? #Resolved

rogancarr · 2019-01-17T17:31:18Z

src/Microsoft.ML.HalLearners/OlsLinearRegression.cs

-        internal OlsLinearRegressionTrainer(IHostEnvironment env, Arguments args)
+        internal OlsLinearRegressionTrainer(IHostEnvironment env, Options args)
            : base(Contracts.CheckRef(env, nameof(env)).Register(LoadNameValue), TrainerUtils.MakeR4VecFeature(args.FeatureColumn),
                  TrainerUtils.MakeR4ScalarColumn(args.LabelColumn), TrainerUtils.MakeR4ScalarWeightColumn(args.WeightColumn, args.WeightColumn.IsExplicit))


I think we don't need the IsExplicit() field here anymore. #Resolved

Yes, i have to rebase to master.

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

rogancarr · 2019-01-17T17:31:38Z

src/Microsoft.ML.HalLearners/OlsLinearRegression.cs

-        /// Initializes a new instance of <see cref="OlsLinearRegressionTrainer"/>
-        /// </summary>
-        internal OlsLinearRegressionTrainer(IHostEnvironment env, Arguments args)
+        internal OlsLinearRegressionTrainer(IHostEnvironment env, Options args)


nit: args => options? #Resolved

rogancarr · 2019-01-17T17:31:59Z

src/Microsoft.ML.HalLearners/OlsLinearRegression.cs

            ShortName = ShortName,
            XmlInclude = new[] { @"<include file='../Microsoft.ML.HalLearners/doc.xml' path='doc/members/member[@name=""OLS""]/*' />" })]
-        public static CommonOutputs.RegressionOutput TrainRegression(IHostEnvironment env, Arguments input)
+        public static CommonOutputs.RegressionOutput TrainRegression(IHostEnvironment env, Options input)


nit: input => options? #Resolved

rogancarr · 2019-01-17T17:33:03Z

src/Microsoft.ML.HalLearners/SymSgdClassificationTrainer.cs


        public override TrainerInfo Info { get; }
-        private readonly Arguments _args;
+        private readonly Options _args;


nit: _args => _options? #Resolved

rogancarr · 2019-01-17T17:33:56Z

src/Microsoft.ML.HalLearners/SymSgdClassificationTrainer.cs

            ShortName = SymSgdClassificationTrainer.ShortName,
            XmlInclude = new[] { @"<include file='../Microsoft.ML.HalLearners/doc.xml' path='doc/members/member[@name=""SymSGD""]/*' />" })]
-        public static CommonOutputs.BinaryClassificationOutput TrainSymSgd(IHostEnvironment env, Arguments input)
+        public static CommonOutputs.BinaryClassificationOutput TrainSymSgd(IHostEnvironment env, Options input)


nit: input => options? #Resolved

rogancarr · 2019-01-17T17:34:10Z

src/Microsoft.ML.HalLearners/SymSgdClassificationTrainer.cs

            EntryPointUtils.CheckInputArgs(host, input);

-            return LearnerEntryPointsUtils.Train<Arguments, CommonOutputs.BinaryClassificationOutput>(host, input,
+            return LearnerEntryPointsUtils.Train<Options, CommonOutputs.BinaryClassificationOutput>(host, input,


nit: ditto #Resolved

justinormont · 2019-01-17T20:44:14Z

PR could use a more descriptive title.

I have to dereference the #1798 pointer to understand or dive into the description instead of just scan the repo's PR titles.

wschin · 2019-01-17T21:51:30Z

src/Microsoft.ML.FastTree/TreeTrainersCatalog.cs

        /// </summary>
        /// <param name="ctx">The <see cref="BinaryClassificationContext"/>.</param>
-        /// <param name="labelColumn">The labelColumn column.</param>
+        /// <param name="labelColumn">The label column.</param>


column [](start = 48, length = 6)

Is it column or column name? #Pending

let me do that on a separate PR; since it touches a lot more files, and is orthogonal to the purpose of this PR.

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

wschin · 2019-01-17T21:52:14Z

src/Microsoft.ML.FastTree/TreeTrainersCatalog.cs

        /// <param name="ctx">The <see cref="BinaryClassificationContext"/>.</param>
-        /// <param name="labelColumn">The labelColumn column.</param>
+        /// <param name="labelColumn">The label column.</param>
        /// <param name="featureColumn">The featureColumn column.</param>


featureColumn [](start = 44, length = 13)

feature vector column's name? #Pending

wschin · 2019-01-17T21:54:16Z

There are many doc strings describe their parameters as a column but they are actually columns' names. Do we want to distinguish column from column name in public APIs? @TomFinley, @eerhardt, any input please? #Pending

sfilipi · 2019-01-17T21:58:19Z

putting up a separate PR for that.
#2177

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

wschin · 2019-01-17T21:59:43Z

src/Microsoft.ML.HalLearners/OlsLinearRegression.cs

    public sealed class OlsLinearRegressionTrainer : TrainerEstimatorBase<RegressionPredictionTransformer<OlsLinearRegressionModelParameters>, OlsLinearRegressionModelParameters>
    {
-        public sealed class Arguments : LearnerInputBaseWithWeight
+        public sealed class Options : LearnerInputBaseWithWeight


Is LearnerInputBaseWithWeights still a good name? Would it be better to use LearnerOptionBaseWithWeight?

think that goes beyond the scope of this PR

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

wschin · 2019-01-17T22:00:23Z

src/Microsoft.ML.HalLearners/OlsLinearRegression.cs

            // incredibly uesful thing to have around.
+            /// <summary>
+            /// L2 regularization weight.
+            /// </summary>


Please merge them into summary. #Resolved

? the comments above?

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

Yep.

// Adding L2 regularization turns this into a form of ridge regression, // rather than, strictly speaking, ordinary least squares. But it is an // incredibly useful thing to have around.

looks more like a doc string.

In reply to: 248858191 [](ancestors = 248858191,248857752)

wschin · 2019-01-17T22:01:08Z

src/Microsoft.ML.HalLearners/OlsLinearRegression.cs

            public float L2Weight = 1e-6f;

+            /// <summary>
+            /// Whether to calculate per parameter significance statistics.


parameter [](start = 41, length = 9)

parameter --> parameter (e.g., the coefficient of the i-th input feature) #Resolved

wschin · 2019-01-17T22:42:32Z

src/Microsoft.ML.HalLearners/HalLearnersCatalog.cs

+        }
+
+        /// <summary>
+        /// Predict a target using a linear regression model trained with the <see cref="OlsLinearRegressionTrainer"/>.


Predict [](start = 12, length = 7)

It doesn't look like this function accepts a feature vector and then produce a float. #Pending

it can accept a feature vector in the options. Is your comment different?

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

wschin · 2019-01-17T23:52:47Z

src/Microsoft.ML.HalLearners/SymSgdClassificationTrainer.cs

+        public sealed class Options : LearnerInputBaseWithLabel
        {
+            /// <summary>
+            /// Degree of lock-free parallelism. Determinism not guaranteed.


Determinism not guaranteed. [](start = 49, length = 27)

not guaranteed if a value grater than one is set. #Resolved

wschin · 2019-01-17T23:53:35Z

src/Microsoft.ML.HalLearners/SymSgdClassificationTrainer.cs

        {
+            /// <summary>
+            /// Degree of lock-free parallelism. Determinism not guaranteed.
+            /// Multi-threading is not supported currently.


Multi-threading is not supported currently. [](start = 16, length = 43)

Does it mean that this argument is not used?

good point! It doesn't seem like it is getting used. I know we switched the parallelism off in the native code, due to MKL being sequential. I'll check if that is the case, and log an issue.

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

wschin · 2019-01-17T23:55:37Z

src/Microsoft.ML.HalLearners/SymSgdClassificationTrainer.cs

            public int NumberOfIterations = 50;

+            /// <summary>
+            /// Tolerance for difference in average loss in consecutive passes.


Please add If the reduction on loss is smaller than the specified tolerance in one iteration, training process will be terminated. #Resolved

wschin · 2019-01-17T23:57:25Z

src/Microsoft.ML.HalLearners/SymSgdClassificationTrainer.cs

            public float Tolerance = 1e-4f;

+            /// <summary>
+            /// Learning rate.


Maybe extend it to Learning rate. A larger value can potentially reduce the training time but incur numerical instability and over-fitting. #Resolved

wschin

Most of my comments are around doc strings. Not sure if you want to address here them but I think they should not block maybe.

Ivanidzo4ka · 2019-01-18T01:54:00Z

src/Microsoft.ML.HalLearners/SymSgdClassificationTrainer.cs

        private TPredictor TrainCore(IChannel ch, RoleMappedData data, LinearModelParameters predictor, int weightSetCount)
        {
            int numFeatures = data.Schema.Feature.Value.Type.GetVectorSize();
            var cursorFactory = new FloatLabelCursor.Factory(data, CursOpt.Label | CursOpt.Features | CursOpt.Weight);


| CursOpt.Weight [](start = 100, length = 16)

You don't accept weight in options, and I don't see any calls to Weight column.
Would be nice to remove it from cursor creation. #Resolved

This PR addresses the estimators inside HalLearners: Two public extension methods, one for simple arguments and the other for advanced options Delete unecessary constructors 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)

sfilipi · 2019-01-18T05:48:12Z

Most of my comments are around doc strings. Not sure if you want to address here them but I think they should not block maybe.

@wschin your feedback is incorporated. I appreciate the time to look at the PR, and the level of detail + suggestions.

Fixing a typo.

sfilipi requested review from TomFinley, abgoswam, artidoro, glebuk and wschin January 17, 2019 06:25

sfilipi self-assigned this Jan 17, 2019

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

sfilipi added this to the 0119 milestone Jan 17, 2019

rogancarr approved these changes Jan 17, 2019

View reviewed changes

sfilipi force-pushed the 1798HalLearners branch from bbcbf71 to 0976ad9 Compare January 17, 2019 18:38

wschin reviewed Jan 17, 2019

View reviewed changes

sfilipi mentioned this pull request Jan 17, 2019

The parameter descriptions should distinguish between columns and column names. #2177

Closed

wschin reviewed Jan 17, 2019

View reviewed changes

wschin approved these changes Jan 17, 2019

View reviewed changes

Ivanidzo4ka reviewed Jan 18, 2019

View reviewed changes

sfilipi added 2 commits January 17, 2019 21:49

Addressing PR issues.

6382274

Fixing a typo.

Post rebase fix, and nit.

00ee8bd

sfilipi added 2 commits January 17, 2019 21:51

addressing @Weschi's comments.

cb84468

post rebase fixes.

5f4095a

sfilipi force-pushed the 1798HalLearners branch from 04ecc89 to 5f4095a Compare January 18, 2019 05:56

sfilipi merged commit b1cc8eb into dotnet:master Jan 18, 2019

sfilipi deleted the 1798HalLearners branch January 18, 2019 06:23

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

Towards #1798 . #2170

Towards #1798 . #2170

Uh oh!

Conversation

sfilipi commented Jan 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rogancarr left a comment

Choose a reason for hiding this comment

Uh oh!

rogancarr Jan 17, 2019 • edited by sfilipi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rogancarr Jan 17, 2019 • edited by sfilipi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rogancarr Jan 17, 2019 • edited by sfilipi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rogancarr Jan 17, 2019 • edited by sfilipi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rogancarr Jan 17, 2019 • edited by sfilipi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rogancarr Jan 17, 2019 • edited by sfilipi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rogancarr Jan 17, 2019 • edited by sfilipi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rogancarr Jan 17, 2019 • edited by sfilipi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rogancarr Jan 17, 2019 • edited by sfilipi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rogancarr Jan 17, 2019 • edited by sfilipi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rogancarr Jan 17, 2019 • edited by sfilipi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinormont commented Jan 17, 2019

Uh oh!

wschin Jan 17, 2019 • edited by sfilipi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wschin Jan 17, 2019 • edited by sfilipi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wschin commented Jan 17, 2019 • edited by sfilipi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sfilipi commented Jan 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sfilipi commented Jan 17, 2019 •

edited

Loading

rogancarr Jan 17, 2019 •

edited by sfilipi

Loading

rogancarr Jan 17, 2019 •

edited by sfilipi

Loading

rogancarr Jan 17, 2019 •

edited by sfilipi

Loading

rogancarr Jan 17, 2019 •

edited by sfilipi

Loading

rogancarr Jan 17, 2019 •

edited by sfilipi

Loading

rogancarr Jan 17, 2019 •

edited by sfilipi

Loading

rogancarr Jan 17, 2019 •

edited by sfilipi

Loading

rogancarr Jan 17, 2019 •

edited by sfilipi

Loading

rogancarr Jan 17, 2019 •

edited by sfilipi

Loading

rogancarr Jan 17, 2019 •

edited by sfilipi

Loading

rogancarr Jan 17, 2019 •

edited by sfilipi

Loading

wschin Jan 17, 2019 •

edited by sfilipi

Loading

wschin Jan 17, 2019 •

edited by sfilipi

Loading

wschin commented Jan 17, 2019 •

edited by sfilipi

Loading

sfilipi commented Jan 17, 2019 •

edited

Loading

wschin Jan 17, 2019 •

edited by sfilipi

Loading

wschin Jan 17, 2019 •

edited by sfilipi

Loading

wschin Jan 17, 2019 •

edited by sfilipi

Loading

sfilipi Jan 18, 2019 •

edited

Loading

wschin Jan 17, 2019 •

edited by sfilipi

Loading

wschin Jan 17, 2019 •

edited by sfilipi

Loading

wschin Jan 17, 2019 •

edited by sfilipi

Loading

Ivanidzo4ka Jan 18, 2019 •

edited by sfilipi

Loading