Scrubbing LogisticRegression learners #2761

abgoswam · 2019-02-27T17:55:31Z

Fixes #2615. Related #2613

Ivanidzo4ka · 2019-02-27T18:01:09Z

        public float L2Weight = Defaults.L2Weight;

does L2Regularization sound better for you? #Resolved

Refers to: src/Microsoft.ML.StandardLearners/Standard/LogisticRegression/LbfgsPredictorBase.cs:28 in 9ea2076. [](commit_id = 9ea2076, deletion_comment = False)

Ivanidzo4ka · 2019-02-27T18:04:08Z

        public int MemorySize = Defaults.MemorySize;

I'm looking on this one, and I think it would be amount of memory be allocated in MB during training.
But in reality it's

/// <summary>
/// Number of previous iterations to remember for estimate of Hessian.
/// </summary>
``` #Resolved

---
Refers to: src/Microsoft.ML.StandardLearners/Standard/LogisticRegression/LbfgsPredictorBase.cs:45 in 9ea2076. [](commit_id = 9ea2076862712cb9856ecaade204d658cf2061f4, deletion_comment = False)

Ivanidzo4ka · 2019-02-27T18:05:07Z

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

@@ -47,7 +47,7 @@ public abstract class OptionsBase : LearnerInputBaseWithWeight
            [Argument(ArgumentType.AtMostOnce, HelpText = "Maximum iterations.", ShortName = "maxiter")]
            [TGUI(Label = "Max Number of Iterations")]
            [TlcModule.SweepableLongParamAttribute("MaxIterations", 1, int.MaxValue)]
-            public int MaxIterations = Defaults.MaxIterations;
+            public int MaximumIterations = Defaults.MaximumIterations;


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

let's stick to NumberOfIterations across all our learners. #Closed

fixed

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

Ivanidzo4ka · 2019-02-27T18:06:34Z

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

@@ -71,7 +71,7 @@ public abstract class OptionsBase : LearnerInputBaseWithWeight
            [Argument(ArgumentType.LastOccurenceWins, HelpText = "Init weights diameter", ShortName = "initwts", SortOrder = 140)]
            [TGUI(Label = "Initial Weights Scale", SuggestedSweeps = "0,0.1,0.5,1")]
            [TlcModule.SweepableFloatParamAttribute("InitWtsDiameter", 0.0f, 1.0f, numSteps: 5)]
-            public float InitWtsDiameter = 0;
+            public float InitialWeightsDiameter = 0;

            // Deprecated


// Deprecated [](start = 12, length = 13)

:)
can you double check it's true? #Resolved

looks like its used... one of our tests MulticlassLogisticRegressionOnnxConversionTest even uses it by setting it to false

i have added a comment

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

My question is it actually used in logisticRegression code?
Across all other learners we just use NumberOfThreads to pass how many threads we want to use.
I just don't see reason why this one should be remain public.

In reply to: 260904491 [](ancestors = 260904491,260874466)

Made internal for now.

I see several usage of this in TestEntryPoint.cs . As you pointed, we should look into whether we can remove this completely and only use NumberOfThreads even from EntryPoints

In reply to: 260905721 [](ancestors = 260905721,260904491,260874466)

Ivanidzo4ka · 2019-02-27T18:09:51Z

        public float L1Weight = Defaults.L1Weight;

L1Regularization ? #Resolved

Refers to: src/Microsoft.ML.StandardLearners/Standard/LogisticRegression/LbfgsPredictorBase.cs:33 in 9ea2076. [](commit_id = 9ea2076, deletion_comment = False)

Ivanidzo4ka · 2019-02-27T18:10:43Z

        public bool ShowTrainingStats = false;

you change it to ShowTrainingStatistics in LogisticRegression #Resolved

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

Ivanidzo4ka · 2019-02-27T18:42:31Z

    public static LogisticRegression LogisticRegression(this BinaryClassificationCatalog.BinaryClassificationTrainers catalog,

I think we trying to have following pattern for trainer names:
{AlgoName}{PredictionKind}Trainer
like above we have
SdcaBinaryTrainer (not a great example tho. it should be StochasticDualCoordinateAscentBinaryTrainer) or LightGBMMulticlassTrainer.

Actually created issue regarding that: #2762 stay tuned!

#Pending

Refers to: src/Microsoft.ML.StandardLearners/StandardLearnersCatalog.cs:427 in 9ea2076. [](commit_id = 9ea2076, deletion_comment = False)

abgoswam · 2019-02-27T18:51:14Z

        public int MemorySize = Defaults.MemorySize;

i will add that as comment ..

in some places its also commented as follows without reference to the Hessian:

/// The number of previous iterations to store

i think its fine to keep the name as MemorySize. what do u think ?

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

Refers to: src/Microsoft.ML.StandardLearners/Standard/LogisticRegression/LbfgsPredictorBase.cs:45 in 9ea2076. [](commit_id = 9ea2076, deletion_comment = False)

Ivanidzo4ka · 2019-02-27T18:54:03Z

        public int MemorySize = Defaults.MemorySize;

I would rather change parameter name to reflect what it does than update documentation.

In reply to: 467984224 [](ancestors = 467984224,467967467)

Refers to: src/Microsoft.ML.StandardLearners/Standard/LogisticRegression/LbfgsPredictorBase.cs:45 in 9ea2076. [](commit_id = 9ea2076, deletion_comment = False)

Ivanidzo4ka · 2019-02-27T18:55:15Z

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

@@ -71,7 +71,7 @@ public abstract class OptionsBase : LearnerInputBaseWithWeight
            [Argument(ArgumentType.LastOccurenceWins, HelpText = "Init weights diameter", ShortName = "initwts", SortOrder = 140)]
            [TGUI(Label = "Initial Weights Scale", SuggestedSweeps = "0,0.1,0.5,1")]
            [TlcModule.SweepableFloatParamAttribute("InitWtsDiameter", 0.0f, 1.0f, numSteps: 5)]
-            public float InitWtsDiameter = 0;
+            public float InitialWeightsDiameter = 0;


InitialWeightsDiameter [](start = 25, length = 22)

So when you do this, you need to take old name and put it in ShortName in ArgumentAttribute like this ShortName="initwts,initwtsDiameter"
#Closed

does it apply to all the other renamings as well ? .

why we adding it to ShortName like that ?

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

Yes

for cmd back compatibility.

In reply to: 260894964 [](ancestors = 260894964,260893400)

abgoswam · 2019-02-27T19:20:40Z

        public float L1Weight = Defaults.L1Weight;

fixed

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

Refers to: src/Microsoft.ML.StandardLearners/Standard/LogisticRegression/LbfgsPredictorBase.cs:33 in 9ea2076. [](commit_id = 9ea2076, deletion_comment = False)

abgoswam · 2019-02-27T19:21:56Z

        public float L2Weight = Defaults.L2Weight;

fixed

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

Refers to: src/Microsoft.ML.StandardLearners/Standard/LogisticRegression/LbfgsPredictorBase.cs:28 in 9ea2076. [](commit_id = 9ea2076, deletion_comment = False)

…iles

abgoswam · 2019-02-28T19:42:50Z

    public static LogisticRegression LogisticRegression(this BinaryClassificationCatalog.BinaryClassificationTrainers catalog,

marking as pending . maybe outside purview of this PR

we should have separate PR for #2762 once names are finalized

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

Refers to: src/Microsoft.ML.StandardLearners/StandardLearnersCatalog.cs:427 in 9ea2076. [](commit_id = 9ea2076, deletion_comment = False)

codecov · 2019-02-28T20:33:09Z

Codecov Report

Merging #2761 into master will increase coverage by 0.03%.
The diff coverage is 84.26%.

@@            Coverage Diff             @@
##           master    #2761      +/-   ##
==========================================
+ Coverage   71.68%   71.72%   +0.03%     
==========================================
  Files         808      809       +1     
  Lines      142402   142491      +89     
  Branches    16113    16116       +3     
==========================================
+ Hits       102076   102195     +119     
+ Misses      35893    35860      -33     
- Partials     4433     4436       +3

Flag	Coverage Δ
#Debug	`71.72% <84.26%> (+0.03%)`	⬆️
#production	`67.95% <77.19%> (+0.02%)`	⬆️
#test	`85.9% <96.87%> (+0.04%)`	⬆️

Impacted Files	Coverage Δ
...L.Mkl.Components/ComputeLRTrainingStdThroughHal.cs	`92.85% <ø> (ø)`	⬆️
...rs/Standard/PoissonRegression/PoissonRegression.cs	`88.57% <ø> (ø)`	⬆️
src/Microsoft.ML.Data/Prediction/Calibrator.cs	`72.67% <ø> (ø)`	⬆️
...StandardLearners/Standard/LinearModelParameters.cs	`60.63% <ø> (ø)`	⬆️
src/Microsoft.ML.StaticPipe/LbfgsStatic.cs	`53.15% <0%> (ø)`	⬆️
....ML.Benchmarks/KMeansAndLogisticRegressionBench.cs	`0% <0%> (ø)`	⬆️
...soft.ML.Tests/PermutationFeatureImportanceTests.cs	`100% <100%> (ø)`	⬆️
...est/Microsoft.ML.StaticPipelineTesting/Training.cs	`99.25% <100%> (ø)`	⬆️
...est/Microsoft.ML.Predictor.Tests/TestPredictors.cs	`63.84% <100%> (ø)`	⬆️
test/Microsoft.ML.Tests/OnnxConversionTest.cs	`97.16% <100%> (ø)`	⬆️
... and 19 more

Ivanidzo4ka · 2019-03-01T23:38:25Z

    protected virtual void GetFeatureContributions(in VBuffer<float> features, ref VBuffer<float> contributions, int top, int bottom, bool normalize)

should be private protected #Closed

Refers to: src/Microsoft.ML.StandardLearners/Standard/LinearModelParameters.cs:265 in 04a4f7a. [](commit_id = 04a4f7a, deletion_comment = False)

Ivanidzo4ka · 2019-03-01T23:42:12Z

        public int MemorySize = Defaults.MemorySize;

Feels a bit too long.
IterationsToRemember, IterationsHistory, IterationsToKeep?

In reply to: 467985202 [](ancestors = 467985202,467984224,467967467)

Refers to: src/Microsoft.ML.StandardLearners/Standard/LogisticRegression/LbfgsPredictorBase.cs:45 in 9ea2076. [](commit_id = 9ea2076, deletion_comment = False)

Ivanidzo4ka · 2019-03-01T23:45:23Z

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


+            /// <summary>
+            /// Run SGD to initialize LR weights, converging to this tolerance.
+            /// </summary>
            [Argument(ArgumentType.AtMostOnce, HelpText = "Run SGD to initialize LR weights, converging to this tolerance",
                ShortName = "sgd")]
            public float SgdInitializationTolerance = 0;


SgdInitializationTolerance [](start = 25, length = 26)

StochasticGradientDescentInitilaizationTolerance ? #Resolved

Ivanidzo4ka · 2019-03-01T23:47:16Z

                    Console.Write(".");

Oh god, It still exist.
this need to be ported to progress channels. #Resolved

Refers to: src/Microsoft.ML.StandardLearners/Standard/LogisticRegression/LbfgsPredictorBase.cs:337 in 04a4f7a. [](commit_id = 04a4f7a, deletion_comment = False)

Ivanidzo4ka · 2019-03-01T23:52:19Z

                    Console.Write(".");

Not part of your PR, so please ignore.

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

Refers to: src/Microsoft.ML.StandardLearners/Standard/LogisticRegression/LbfgsPredictorBase.cs:337 in 04a4f7a. [](commit_id = 04a4f7a, deletion_comment = False)

Ivanidzo4ka · 2019-03-03T23:44:43Z

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

-            MaxIterations = LbfgsTrainerOptions.MaxIterations;
-            SgdInitializationTolerance = LbfgsTrainerOptions.SgdInitializationTolerance;
+            Host.CheckUserArg(!LbfgsTrainerOptions.UseThreads || LbfgsTrainerOptions.NumberOfThreads > 0 || LbfgsTrainerOptions.NumberOfThreads == null,
+              nameof(LbfgsTrainerOptions.NumberOfThreads), "numThreads must be positive (or empty for default)");


numThreads [](start = 60, length = 10)

Must be positive, if provided.

Ivanidzo4ka · 2019-03-03T23:53:12Z

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

+            /// If set to <value>true</value>training statistics will be generated at the end of training.
+            /// </summary>
+            [Argument(ArgumentType.AtMostOnce, HelpText = "Show statistics of training examples.", ShortName = "stat, ShowTrainingStats", SortOrder = 50)]
+            public bool ShowTrainingStatistics = false;


ShowTrainingStatistics [](start = 24, length = 22)

Can you double check it's used?
It strange binary LR and MultiLR has different options (binary LR has extra thing ComputeLogisticRegressionStandardDeviation)

I do see it being used for both binary LR, and multiclass LR

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

Lines 461 to 462 in df2f3fb

if (ShowTrainingStats)

ProcessPriorDistribution(cursor.Label, cursor.Weight);

and here

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

Lines 567 to 568 in df2f3fb

if (ShowTrainingStats)

ComputeTrainingStatistics(ch, cursorFactory, loss, numParams);

Binary LR has the additional ComputeLogisticRegressionStandardDeviation as you noted

Ivanidzo4ka

artidoro · 2019-03-04T18:24:20Z

public abstract class LbfgsTrainerBase<TOptions, TTransformer, TModel> : TrainerEstimatorBase<TTransformer, TModel>

Should we rname this without acronyms? #Resolved

Refers to: src/Microsoft.ML.StandardLearners/Standard/LogisticRegression/LbfgsPredictorBase.cs:18 in 7a7ac78. [](commit_id = 7a7ac78, deletion_comment = False)

abgoswam · 2019-03-04T18:28:24Z

public abstract class LbfgsTrainerBase<TOptions, TTransformer, TModel> : TrainerEstimatorBase<TTransformer, TModel>

There is a separate issue #2762 from trainer names..maybe we can address changes (if any) as part of #2762

the scrubbing PRs are focussed on the items mentioned in #2613

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

Refers to: src/Microsoft.ML.StandardLearners/Standard/LogisticRegression/LbfgsPredictorBase.cs:18 in 7a7ac78. [](commit_id = 7a7ac78, deletion_comment = False)

artidoro · 2019-03-04T18:32:10Z

public abstract class LbfgsTrainerBase<TOptions, TTransformer, TModel> : TrainerEstimatorBase<TTransformer, TModel>

I see "no short names" in the items mentioned in #2613. I thought it was part of the scrubbing tasks?

In reply to: 469362212 [](ancestors = 469362212,469360727)

Refers to: src/Microsoft.ML.StandardLearners/Standard/LogisticRegression/LbfgsPredictorBase.cs:18 in 7a7ac78. [](commit_id = 7a7ac78, deletion_comment = False)

artidoro · 2019-03-04T18:41:34Z

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

-            [Argument(ArgumentType.AtMostOnce, HelpText = "Show statistics of training examples.", ShortName = "stat", SortOrder = 50)]
-            public bool ShowTrainingStats = false;
+            /// <summary>
+            /// If set to <value>true</value>training statistics will be generated at the end of training.


Could you add a space before training?

abgoswam · 2019-03-04T18:55:23Z

public abstract class LbfgsTrainerBase<TOptions, TTransformer, TModel> : TrainerEstimatorBase<TTransformer, TModel>

please note : "no short names" is for Options , not trainer names

(at least thats what i am following. keeping trainer names for #2762)

In reply to: 469364309 [](ancestors = 469364309,469362212,469360727)

Refers to: src/Microsoft.ML.StandardLearners/Standard/LogisticRegression/LbfgsPredictorBase.cs:18 in 7a7ac78. [](commit_id = 7a7ac78, deletion_comment = False)

artidoro · 2019-03-04T18:56:49Z

public abstract class LbfgsTrainerBase<TOptions, TTransformer, TModel> : TrainerEstimatorBase<TTransformer, TModel>

OK I see!

In reply to: 469376815 [](ancestors = 469376815,469364309,469362212,469360727)

Refers to: src/Microsoft.ML.StandardLearners/Standard/LogisticRegression/LbfgsPredictorBase.cs:18 in 7a7ac78. [](commit_id = 7a7ac78, deletion_comment = False)

artidoro · 2019-03-04T18:57:15Z

public abstract class LbfgsTrainerBase<TOptions, TTransformer, TModel> : TrainerEstimatorBase<TTransformer, TModel>

Sorry for this then

In reply to: 469377354 [](ancestors = 469377354,469376815,469364309,469362212,469360727)

Refers to: src/Microsoft.ML.StandardLearners/Standard/LogisticRegression/LbfgsPredictorBase.cs:18 in 7a7ac78. [](commit_id = 7a7ac78, deletion_comment = False)

artidoro

abgoswam · 2019-03-04T18:59:01Z

public abstract class LbfgsTrainerBase<TOptions, TTransformer, TModel> : TrainerEstimatorBase<TTransformer, TModel>

no worries at all .. thanks for noticing actually :)

In reply to: 469377514 [](ancestors = 469377514,469377354,469376815,469364309,469362212,469360727)

Refers to: src/Microsoft.ML.StandardLearners/Standard/LogisticRegression/LbfgsPredictorBase.cs:18 in 7a7ac78. [](commit_id = 7a7ac78, deletion_comment = False)

LogisticRegression

9ea2076

Ivanidzo4ka reviewed Feb 27, 2019

View reviewed changes

LogisticRegression - 2

e49ca59

abgoswam added 3 commits February 28, 2019 17:53

added ShortName for entrypoint back compatibility; updated manifest f…

e805900

…iles

also looked into MultiClass LR and PoissonRegression

2b3f88f

merge with latest master

74f038d

abgoswam changed the title ~~WIP : Scrubbing LogisticRegression learners~~ Scrubbing LogisticRegression learners Feb 28, 2019

review comments

04a4f7a

Ivanidzo4ka requested review from zeahmed and wschin March 1, 2019 23:36

Ivanidzo4ka reviewed Mar 1, 2019

View reviewed changes

abgoswam added 2 commits March 3, 2019 03:33

Merge branch 'master' into abgoswam/scrub_LR

a0c7a90

review comments

7a7ac78

Ivanidzo4ka reviewed Mar 3, 2019

View reviewed changes

Ivanidzo4ka approved these changes Mar 3, 2019

View reviewed changes

abgoswam requested a review from artidoro March 4, 2019 18:12

artidoro reviewed Mar 4, 2019

View reviewed changes

artidoro approved these changes Mar 4, 2019

View reviewed changes

abgoswam added 3 commits March 4, 2019 19:18

review comments

c46837d

merge with master

43baa27

ensure MLcontext names are same as those used in Options

064d364

abgoswam merged commit 24926ee into dotnet:master Mar 4, 2019

abgoswam deleted the abgoswam/scrub_LR branch March 20, 2019 20:13

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

	if (ShowTrainingStats)
	ProcessPriorDistribution(cursor.Label, cursor.Weight);

	if (ShowTrainingStats)
	ComputeTrainingStatistics(ch, cursorFactory, loss, numParams);

Scrubbing LogisticRegression learners #2761

Scrubbing LogisticRegression learners #2761

Conversation

abgoswam commented Feb 27, 2019 • edited Loading

Ivanidzo4ka commented Feb 27, 2019 • edited by abgoswam Loading

Ivanidzo4ka commented Feb 27, 2019 • edited by abgoswam Loading

Ivanidzo4ka Feb 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Feb 27, 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

Ivanidzo4ka commented Feb 27, 2019 • edited by abgoswam Loading

Ivanidzo4ka commented Feb 27, 2019 • edited by abgoswam Loading

Ivanidzo4ka commented Feb 27, 2019 • edited by abgoswam Loading

abgoswam commented Feb 27, 2019 • edited Loading

Ivanidzo4ka commented Feb 27, 2019

Ivanidzo4ka Feb 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abgoswam commented Feb 27, 2019

abgoswam commented Feb 27, 2019

abgoswam commented Feb 28, 2019

codecov bot commented Feb 28, 2019 • edited Loading

Codecov Report

Ivanidzo4ka commented Mar 1, 2019 • edited Loading

Ivanidzo4ka commented Mar 1, 2019

Ivanidzo4ka Mar 1, 2019 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka commented Mar 1, 2019 • edited Loading

Ivanidzo4ka commented Mar 1, 2019

Ivanidzo4ka Mar 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

artidoro commented Mar 4, 2019 • edited Loading

abgoswam commented Mar 4, 2019

artidoro commented Mar 4, 2019

Choose a reason for hiding this comment

abgoswam commented Mar 4, 2019 • edited Loading

artidoro commented Mar 4, 2019

artidoro commented Mar 4, 2019

artidoro left a comment

Choose a reason for hiding this comment

abgoswam commented Mar 4, 2019

abgoswam commented Feb 27, 2019 •

edited

Loading

Ivanidzo4ka commented Feb 27, 2019 •

edited by abgoswam

Loading

Ivanidzo4ka commented Feb 27, 2019 •

edited by abgoswam

Loading

Ivanidzo4ka Feb 27, 2019 •

edited

Loading

Ivanidzo4ka Feb 27, 2019 •

edited by abgoswam

Loading

Ivanidzo4ka commented Feb 27, 2019 •

edited by abgoswam

Loading

Ivanidzo4ka commented Feb 27, 2019 •

edited by abgoswam

Loading

Ivanidzo4ka commented Feb 27, 2019 •

edited by abgoswam

Loading

abgoswam commented Feb 27, 2019 •

edited

Loading

Ivanidzo4ka Feb 27, 2019 •

edited

Loading

codecov bot commented Feb 28, 2019 •

edited

Loading

Ivanidzo4ka commented Mar 1, 2019 •

edited

Loading

Ivanidzo4ka Mar 1, 2019 •

edited

Loading

Ivanidzo4ka commented Mar 1, 2019 •

edited

Loading

Ivanidzo4ka Mar 3, 2019 •

edited

Loading

artidoro commented Mar 4, 2019 •

edited

Loading

abgoswam commented Mar 4, 2019 •

edited

Loading