Conversion of Logistic, Multiclass Logistic, and Poisson Regression to estimators by artidoro · Pull Request #957 · dotnet/machinelearning

artidoro · 2018-09-19T22:50:10Z

Fixes #956.

Conversion of Logistic, Multiclass Logistic, and Poisson Regression to estimators by deriving the base class LbfgsTrainerBase from TrainerEstimatorBase.

Working on testing the converted classes using TestEstimatorCore.

sfilipi · 2018-09-20T06:10:50Z

add a second constructor taking in the label, feature, weight column. #Closed

Refers to: src/Microsoft.ML.StandardLearners/Standard/PoissonRegression/PoissonRegression.cs:47 in c7a996e. [](commit_id = c7a996e, deletion_comment = False)

sfilipi · 2018-09-20T06:11:25Z

add the other constructor. #Closed

Refers to: src/Microsoft.ML.StandardLearners/Standard/LogisticRegression/MulticlassLogisticRegression.cs:76 in c7a996e. [](commit_id = c7a996e, deletion_comment = False)

sfilipi · 2018-09-20T06:11:50Z

+            {
+                new SchemaShape.Column(DefaultColumnNames.Score, SchemaShape.Column.VectorKind.Scalar, NumberType.R4, false, new SchemaShape(MetadataUtils.GetTrainerOutputMetadata())),
+                new SchemaShape.Column(DefaultColumnNames.Probability, SchemaShape.Column.VectorKind.Scalar, NumberType.R4, false, new SchemaShape(MetadataUtils.GetTrainerOutputMetadata(true))),
+                new SchemaShape.Column(DefaultColumnNames.PredictedLabel, SchemaShape.Column.VectorKind.Scalar, BoolType.Instance, false, new SchemaShape(MetadataUtils.GetTrainerOutputMetadata()))


don;t think we get probability for multi-class #Closed

actually Score is the probability vector for the case of multiclass.

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

sfilipi · 2018-09-20T06:15:05Z

+            {
+                new SchemaShape.Column(DefaultColumnNames.Score, SchemaShape.Column.VectorKind.Scalar, NumberType.R4, false, new SchemaShape(MetadataUtils.GetTrainerOutputMetadata())),
+                new SchemaShape.Column(DefaultColumnNames.Probability, SchemaShape.Column.VectorKind.Scalar, NumberType.R4, false, new SchemaShape(MetadataUtils.GetTrainerOutputMetadata(true))),
+                new SchemaShape.Column(DefaultColumnNames.PredictedLabel, SchemaShape.Column.VectorKind.Scalar, BoolType.Instance, false, new SchemaShape(MetadataUtils.GetTrainerOutputMetadata()))


only score. #Closed

Zruty0 · 2018-09-20T19:23:15Z

    public LogisticRegression(IHostEnvironment env, Arguments args)

let's add a ctor that has Action<args> #Resolved

Refers to: src/Microsoft.ML.StandardLearners/Standard/LogisticRegression/LogisticRegression.cs:52 in c7a996e. [](commit_id = c7a996e, deletion_comment = False)

Zruty0 · 2018-09-20T19:23:34Z

    public LogisticRegression(IHostEnvironment env, Arguments args)

and the necessary column values

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

Refers to: src/Microsoft.ML.StandardLearners/Standard/LogisticRegression/LogisticRegression.cs:52 in c7a996e. [](commit_id = c7a996e, deletion_comment = False)

Zruty0 · 2018-09-20T19:24:27Z


        internal LbfgsTrainerBase(ArgumentsBase args, IHostEnvironment env, string name, bool showTrainingStats = false)
-            : base(env, name)
+            : base(Contracts.CheckRef(env, nameof(env)).Register(name), MakeFeatureColumn(args.FeatureColumn), MakeFeatureColumn(args.LabelColumn), MakeWeightColumn(args.WeightColumn))


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

I'd pass feature, label and weights separately from args #Resolved

Zruty0 · 2018-09-20T19:24:49Z

tests that contain TestEstimatorCore please #Resolved

sfilipi · 2018-09-20T21:23:27Z

+
+namespace Microsoft.ML.Tests
+{
+    public class LbfgsTests : TestDataPipeBase


class Lbfgs [](start = 11, length = 11)

if you sync to latest, you can make it a TrainerEstimators partial class. #Closed

sfilipi · 2018-09-20T21:23:48Z

+    {
+        private IDataView GetIrisDataview()
+        {
+            var dataPath = GetDataPath("iris.txt");


"iris.txt") [](start = 39, length = 11)

imo, tend to use the TestDatasets classes #Closed

sfilipi · 2018-09-20T21:25:06Z

+        public void TestEstimatorLogisticRegression()
+        {
+            var dataView = GetBreastCancerDataview();
+            dataView = Env.CreateTransform("Term{col=F1}", dataView);


Env.CreateTransform("Term{col=F1}", dataView); [](start = 23, length = 46)

should aim at not using maml #Closed

There are utility methods to get pipelines for tasks on the LightGBM PR. maybe they'll be useful.

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

Thank you! I'll look into changing this then.

In reply to: 219323059 [](ancestors = 219323059,219322724)

sfilipi · 2018-09-20T21:37:24Z

+            string groupIdColumn = null, string weightColumn = null, Action<Arguments> advancedSettings = null)
+            : base(env, featureColumn, labelColumn, weightColumn, groupIdColumn, advancedSettings)
+        {
+        }


add Host.CheckNonEmpty for label, feature #Closed

sfilipi · 2018-09-20T21:38:30Z

+            return new[]
+            {
+                new SchemaShape.Column(DefaultColumnNames.Score, SchemaShape.Column.VectorKind.Scalar, NumberType.R4, false, new SchemaShape(MetadataUtils.GetTrainerOutputMetadata())),
+                new SchemaShape.Column(DefaultColumnNames.PredictedLabel, SchemaShape.Column.VectorKind.Scalar, BoolType.Instance, false, new SchemaShape(MetadataUtils.GetTrainerOutputMetadata()))


new SchemaSh [](start = 138, length = 12)

look at what SDCA multiclass is doing for the metadata append the Label metadata here. #Closed

sfilipi · 2018-09-20T21:39:00Z


+        public MulticlassLogisticRegression(IHostEnvironment env, string featureColumn, string labelColumn,
+            string groupIdColumn = null, string weightColumn = null, Action<Arguments> advancedSettings = null)
+            : base(env, featureColumn, labelColumn, weightColumn, groupIdColumn, advancedSettings)


labelColumn [](start = 39, length = 11)

shape of the label column for multi-class is different than regression, will want to construct it here. #Closed

I kept it as it is now and the tests seem to be passing correctly. Am I missing something?

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

i think the label for multiclass is a key, and item type U4. R4 is probably acceptable, but you need it to be a key.

In reply to: 219349397 [](ancestors = 219349397,219326153)

sfilipi · 2018-09-20T21:41:09Z

+            NumThreads = Args.NumThreads;
+            DenseOptimizer = Args.DenseOptimizer;
+            //ShowTrainingStats = showTrainingStats; // TODO
+            EnforceNonNegativity = Args.EnforceNonNegativity;


try to avoid repetition between the two constructors #Closed

sfilipi · 2018-09-20T21:42:40Z

+            var dataView = GetIrisDataview();
+            var data = FeatureCombiner.PrepareFeatures(Env, new FeatureCombiner.FeatureCombinerInput() { Data = dataView, Features = new[] { "SepalLength", "SepalWidth", "PetalLength", "PetalWidth" } }).OutputData;
+
+            var pipe = new LogisticRegression(Env, "Features", "Label");


LogisticRegression [](start = 27, length = 18)

did you mean multiclassLR? #Closed

sfilipi · 2018-09-20T21:42:47Z

+        public void TestEstimatorPoissonRegression()
+        {
+            var dataView = GetGeneratedRegressionDataview();
+            var pipe = new LogisticRegression(Env, "Features", "Label");


LogisticRegression [](start = 27, length = 18)

Possion #Closed

…to lrandmultilr

zeahmed · 2018-09-21T00:17:43Z

+        }
+
+        internal LbfgsTrainerBase(IHostEnvironment env, TArgs args)
+            : base(Contracts.CheckRef(env, nameof(env)).Register(RegisterName), MakeFeatureColumn(args.FeatureColumn), MakeFeatureColumn(args.LabelColumn), MakeWeightColumn(args.WeightColumn))


MakeFeatureColumn [](start = 119, length = 17)

MakeLabelColumn? #Closed

zeahmed · 2018-09-21T00:24:14Z

+        public void TestEstimatorLogisticRegression()
+        {
+            var dataView = GetBreastCancerDataview();
+            //var args = new LogisticRegression.Arguments();


//var args = new LogisticRegression.Arguments(); [](start = 12, length = 48)

please remove commented code. #Closed

zeahmed · 2018-09-21T00:27:21Z

+            InitArguments();
+        }
+
+        internal LbfgsTrainerBase(IHostEnvironment env, TArgs args)


LbfgsTrainerBase [](start = 17, length = 16)

I think you can call this constructor from the other one and make all the fields back to readonly #Closed

That might be a bit hard. The arguments would not be in Args before running the code in the original constructor. And I believe that happens after calling the other constructor.

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

I'll try to keep them readonly though.

In reply to: 219593965 [](ancestors = 219593965,219355643)

Actually for TAgrs, you could write a static method that would return the TArgs and that you could use in this() call as this(...CreateArgs(...)..).

In reply to: 219594177 [](ancestors = 219594177,219593965,219355643)

Oh that's a good idea! I will look into that.

In reply to: 219653751 [](ancestors = 219653751,219594177,219593965,219355643)

sfilipi · 2018-09-21T04:58:23Z

+            InitArguments();
+        }
+
+        protected void InitArguments()


protected [](start = 8, length = 9)

would private suffice? #Closed

sfilipi · 2018-09-24T21:24:41Z

+            UseThreads = Args.UseThreads;
+            NumThreads = Args.NumThreads;
+            DenseOptimizer = Args.DenseOptimizer;
+            EnforceNonNegativity = Args.EnforceNonNegativity;


sorry just notice that those are gone from the constructor. You still need them; you can remove readonly and set them in a method, call that from both constructors. #Closed

They are been called by both constructors. I set it up so that the first constructor calls the second using this(...). It was suggested by Zeeshan A.

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

ah, missed.

In reply to: 220022095 [](ancestors = 220022095,219994227)

sfilipi · 2018-09-24T21:27:53Z

+        /// <summary>
+        /// Initializes a new instance of <see cref="MulticlassLogisticRegression"/>
+        /// </summary>
        public MulticlassLogisticRegression(IHostEnvironment env, Arguments args)


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

is internal enough? #Closed

Ivanidzo4ka · 2018-09-25T00:04:51Z

@@ -27,7 +26,7 @@
 namespace Microsoft.ML.Runtime.Learners
 {
    /// <include file='doc.xml' path='doc/members/member[@name="PoissonRegression"]/*' />


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

If you look in doc.xml in our code sample we have this:

new PoissonRegressor { MaxIterations = 100, L2Weight = 0.6f }

Which is no longer are valid statement. #ByDesign

Actually I think it's a much broader issue, and need to be resolved for all converted to estimators classes.

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

Ivanidzo4ka · 2018-09-25T00:07:08Z

    /// <include file = 'doc.xml' path='doc/members/member[@name="LBFGS"]/*' />
    /// <include file = 'doc.xml' path='docs/members/example[@name="LogisticRegressionClassifier"]/*' />
-    public sealed class MulticlassLogisticRegression : LbfgsTrainerBase<VBuffer<Float>, MulticlassLogisticRegressionPredictor>
+    public sealed class MulticlassLogisticRegression : LbfgsTrainerBase<MulticlassLogisticRegression.Arguments, MulticlassPredictionTransformer<MulticlassLogisticRegressionPredictor>, MulticlassLogisticRegressionPredictor>


can you break this line? #Closed

Ivanidzo4ka · 2018-09-25T00:10:26Z

            // MeanImprovementCriterion:
            //   Terminates when the geometrically-weighted average improvement falls below the tolerance
-            terminationCriterion = new MeanImprovementCriterion(OptTol, (Float)0.25, MaxIterations);
+            terminationCriterion = new MeanImprovementCriterion(OptTol, (float)0.25, MaxIterations);


(float)0.25 [](start = 72, length = 11)

0.25f
no reason to cast it. #Closed

Ivanidzo4ka · 2018-09-25T00:12:34Z

+        {
+            var cols = new List<SchemaShape.Column>();
+            foreach (SchemaShape.Column col in MetadataUtils.GetTrainerOutputMetadata())
+            {


{ [](start = 12, length = 1)

I think we have tendency to omit braces for one liners. #Closed

Actually, why can't you do cols.AddRange(MetadataUtils.GetTrainerOutputMetadata()) ?

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

Thanks I think that that works!!

In reply to: 220026684 [](ancestors = 220026684,220026583)

Ivanidzo4ka · 2018-09-25T00:16:18Z

+        private static int NonZeroCount(ref VBuffer<float> vector)
        {
            if (!vector.IsDense)
                return vector.Count;


Not related to your changes, but I think this is wrong.
Also, great review comment for this method! #Resolved

I do think it's a pretty extreme review comment! haha

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

Should we file an issue on this?

In reply to: 220044369 [](ancestors = 220044369,220027114)

Ivanidzo4ka · 2018-09-25T00:17:59Z

        writer.WriteLine("MulticlassLogisticRegression bias and non-zero weights");

can you use nameof(MulticlassLogisticRegression) instead? #Resolved

Refers to: src/Microsoft.ML.StandardLearners/Standard/LogisticRegression/MulticlassLogisticRegression.cs:790 in 4dcb4ab. [](commit_id = 4dcb4ab, deletion_comment = False)

zeahmed

zeahmed

Ivanidzo4ka · 2018-09-25T00:20:20Z

            public bool EnforceNonNegativity = false;
        }

+        private const string RegisterName = "LbfgsTraining";


RegisterName [](start = 29, length = 12)

you can use nameof(LbfgsTrainerBase) in code instead of this constant. #Closed

Ivanidzo4ka · 2018-09-25T00:23:04Z

                for (int j = 0; j < initWeights.Length; j++)
-                    initWeights[j] = InitWtsDiameter * (Host.Rand.NextSingle() - (Float)0.5);
-                init = new VBuffer<Float>(initWeights.Length, initWeights);
+                    initWeights[j] = InitWtsDiameter * (Host.Rand.NextSingle() - (float)0.5);


(float)0.5 [](start = 81, length = 10)

0.5f, no reason for cast. #Closed

Ivanidzo4ka

Ivanidzo4ka

Ivanidzo4ka · 2018-09-25T16:55:27Z

        writer.WriteLine("MulticlassLogisticRegression bias and non-zero weights");

I don't meant to remove "bias and non-zero weights" from output, just replace string constant of class to nameof class.

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

Refers to: src/Microsoft.ML.StandardLearners/Standard/LogisticRegression/MulticlassLogisticRegression.cs:790 in 4dcb4ab. [](commit_id = 4dcb4ab, deletion_comment = False)

artidoro · 2018-09-25T17:24:51Z

        writer.WriteLine("MulticlassLogisticRegression bias and non-zero weights");

Sorry I didn't understood right away. I just fixed it.

In reply to: 424420860 [](ancestors = 424420860,424166489)

Refers to: src/Microsoft.ML.StandardLearners/Standard/LogisticRegression/MulticlassLogisticRegression.cs:790 in 4dcb4ab. [](commit_id = 4dcb4ab, deletion_comment = False)

Ivanidzo4ka · 2018-09-25T18:42:40Z

Please merge it with master. #Resolved

…to lrandmultilr

derived from trainerestimatorbase

31c888b

artidoro added the api label Sep 19, 2018

artidoro added 2 commits September 19, 2018 15:54

cleaned up

65a5098

sorting namespaces

c7a996e

sfilipi reviewed Sep 20, 2018

View reviewed changes

Zruty0 reviewed Sep 20, 2018

View reviewed changes

fixed review comments, still some more features to add

95f666a

sfilipi reviewed Sep 20, 2018

View reviewed changes

artidoro added 4 commits September 20, 2018 14:58

pr comments on tests

eb39062

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

a34d842

…to lrandmultilr

updated the tests

c250d21

fixed review comments

6b9791e

zeahmed reviewed Sep 21, 2018

View reviewed changes

sfilipi reviewed Sep 21, 2018

View reviewed changes

sfilipi reviewed Sep 24, 2018

View reviewed changes

artidoro added 2 commits September 24, 2018 16:39

cleaned up code and fixed documentation text

94b58cc

fixed review comments

4dcb4ab

Ivanidzo4ka reviewed Sep 25, 2018

View reviewed changes

zeahmed approved these changes Sep 25, 2018

View reviewed changes

Ivanidzo4ka reviewed Sep 25, 2018

View reviewed changes

Ivanidzo4ka approved these changes Sep 25, 2018

View reviewed changes

artidoro added 2 commits September 24, 2018 19:57

fixed review comments

8ba8eb6

fixed review comments

7cc6267

sfilipi assigned artidoro Sep 25, 2018

sfilipi approved these changes Sep 25, 2018

View reviewed changes

Ivanidzo4ka approved these changes Sep 25, 2018

View reviewed changes

fized review comment

8cb5cec

artidoro added 3 commits September 25, 2018 11:52

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

9da48f9

…to lrandmultilr

fixed more review comments

518116c

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

4fb2013

…to lrandmultilr

artidoro merged commit c0545a7 into dotnet:master Sep 25, 2018

artidoro deleted the lrandmultilr branch January 5, 2019 00:02

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

Conversation

artidoro commented Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sfilipi commented Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sfilipi commented Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sfilipi Sep 20, 2018 • edited 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!

sfilipi Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zruty0 commented Sep 20, 2018 • edited by artidoro Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zruty0 commented Sep 20, 2018

Uh oh!

Zruty0 Sep 20, 2018 • edited by artidoro Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zruty0 commented Sep 20, 2018 • edited by artidoro Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sfilipi Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfilipi Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfilipi Sep 20, 2018 • edited 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!

Choose a reason for hiding this comment

Uh oh!

sfilipi Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfilipi Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfilipi Sep 20, 2018 • edited 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!

Choose a reason for hiding this comment

Uh oh!

sfilipi Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfilipi Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfilipi Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeahmed Sep 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artidoro commented Sep 19, 2018 •

edited

Loading

sfilipi commented Sep 20, 2018 •

edited

Loading

sfilipi commented Sep 20, 2018 •

edited

Loading

sfilipi Sep 20, 2018 •

edited

Loading

sfilipi Sep 20, 2018 •

edited

Loading

Zruty0 commented Sep 20, 2018 •

edited by artidoro

Loading

Zruty0 Sep 20, 2018 •

edited by artidoro

Loading

Zruty0 commented Sep 20, 2018 •

edited by artidoro

Loading

sfilipi Sep 20, 2018 •

edited

Loading

sfilipi Sep 20, 2018 •

edited

Loading

sfilipi Sep 20, 2018 •

edited

Loading

sfilipi Sep 20, 2018 •

edited

Loading

sfilipi Sep 20, 2018 •

edited

Loading

sfilipi Sep 20, 2018 •

edited

Loading

sfilipi Sep 20, 2018 •

edited

Loading

sfilipi Sep 20, 2018 •

edited

Loading

sfilipi Sep 20, 2018 •

edited

Loading

zeahmed Sep 21, 2018 •

edited

Loading

zeahmed Sep 21, 2018 •

edited

Loading

zeahmed Sep 21, 2018 •

edited

Loading

sfilipi Sep 21, 2018 •

edited

Loading

sfilipi Sep 24, 2018 •

edited

Loading

sfilipi Sep 24, 2018 •

edited

Loading

Ivanidzo4ka Sep 25, 2018 •

edited

Loading

Ivanidzo4ka Sep 25, 2018 •

edited

Loading

Ivanidzo4ka Sep 25, 2018 •

edited

Loading

Ivanidzo4ka Sep 25, 2018 •

edited

Loading