One type label policy in trainers #2804

Ivanidzo4ka · 2019-02-28T23:17:51Z

This PR fixes #2628 and fixes #2750. fixes #2810

codecov · 2019-03-02T02:17:32Z

Codecov Report

Merging #2804 into master will increase coverage by 0.02%.
The diff coverage is 82.27%.

@@            Coverage Diff             @@
##           master    #2804      +/-   ##
==========================================
+ Coverage   71.81%   71.83%   +0.02%     
==========================================
  Files         812      812              
  Lines      142658   142708      +50     
  Branches    16095    16092       -3     
==========================================
+ Hits       102445   102512      +67     
+ Misses      35830    35818      -12     
+ Partials     4383     4378       -5

Flag	Coverage Δ
#Debug	`71.83% <82.27%> (+0.02%)`	⬆️
#production	`67.97% <65.3%> (+0.01%)`	⬆️
#test	`86.25% <89.9%> (ø)`	⬆️

Impacted Files	Coverage Δ
...oft.ML.StandardTrainers/Standard/SdcaMultiClass.cs	`93.51% <ø> (+1.26%)`	⬆️
src/Microsoft.ML.FastTree/FastTreeRanking.cs	`48.19% <ø> (ø)`	⬆️
...crosoft.ML.StandardTrainers/Standard/SdcaBinary.cs	`72.68% <ø> (+0.13%)`	⬆️
...dardTrainers/Standard/Online/AveragedPerceptron.cs	`89.7% <ø> (+3.99%)`	⬆️
...st/Microsoft.ML.Benchmarks.Tests/BenchmarksTest.cs	`0% <ø> (ø)`	⬆️
...s/StochasticDualCoordinateAscentClassifierBench.cs	`0% <0%> (ø)`	⬆️
src/Microsoft.ML.Core/Data/AnnotationUtils.cs	`81.77% <0%> (-0.53%)`	⬇️
...t/Microsoft.ML.Benchmarks/PredictionEngineBench.cs	`0% <0%> (ø)`	⬆️
...soft.ML.Data/Scorers/MultiClassClassifierScorer.cs	`59.64% <0%> (ø)`	⬆️
test/Microsoft.ML.TestFramework/Datasets.cs	`100% <100%> (ø)`	⬆️
... and 21 more

rogancarr · 2019-03-04T20:07:02Z

namespace Microsoft.ML.Trainers.FastTree

FastTree and GAM binary classification convert from R4 to boolean with a custom function. Do we want to drop that functionality?

Refers to: src/Microsoft.ML.FastTree/FastTreeRanking.cs:40 in 5707e17. [](commit_id = 5707e17, deletion_comment = False)

rogancarr · 2019-03-04T20:13:16Z

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

@@ -20,21 +20,23 @@ public void SdcaWorkout()
            var data = TextLoaderStatic.CreateLoader(Env, ctx => (Label: ctx.LoadFloat(0), Features: ctx.LoadFloat(1, 10)))
                .Load(dataPath).Cache();

+            var binaryData = ML.Transforms.Conversion.ConvertType("Label", outputKind: DataKind.Boolean).Fit(data.AsDynamic).Transform(data.AsDynamic);


var binaryData [](start = 11, length = 15)

Maybe break lines? This is a bit of a mouthful. #Resolved

rogancarr · 2019-03-04T20:13:50Z

test/Microsoft.ML.Tests/Scenarios/OvaTest.cs

            // Data
-            var data = mlContext.Data.Cache(reader.Load(GetDataPath(dataPath)));
+            var textData = reader.Load(GetDataPath(dataPath));
+            var data = mlContext.Data.Cache(mlContext.Transforms.Conversion.MapValueToKey("Label").Fit(textData).Transform(textData));


var data [](start = 12, length = 9)

Maybe for lines like this in this file, break the steps out a bit for easier reading? #Resolved

rogancarr · 2019-03-04T20:14:14Z

test/Microsoft.ML.Tests/Scenarios/OvaTest.cs

            var pipeline = mlContext.MulticlassClassification.Trainers.OneVersusAll(ap, useProbabilities: false);

            var model = pipeline.Fit(data);
            var predictions = model.Transform(data);

            // Metrics
            var metrics = mlContext.MulticlassClassification.Evaluate(predictions);
-            Assert.True(metrics.MicroAccuracy > 0.71);
+            Assert.True(metrics.MicroAccuracy > 0.66);


Assert.True(metrics.MicroAccuracy > 0.66) [](start = 12, length = 41)

Did changing the concurrency give lower MicroAccuracy?

No.
I still need to debug it properly.
We have ova with 3 labels which gives me 0 vs 1,2; 1 vs 0,2 and 2 vs 0,1.
For 1 vs 0,2 AP with 1 iteration (which is default) treats everything as 0,2. I've even modify file and relabel it accordingly to test that. For other cases AP perfectly overfits and give you correct predictions on training data. Which leads to 2/3 accuracy.
For some reason what wasn't the case before with float labels, as I said, I need to look on that.
Concurency =1 set for ease of debugging, I can remove it.

rogancarr · 2019-03-04T20:15:10Z

src/Microsoft.ML.Core/Data/AnnotationUtils.cs

@@ -307,11 +307,11 @@ public static void GetSlotNames(RoleMappedSchema schema, RoleMappedSchema.Column
                schema.Schema[list[0].Index].Annotations.GetValue(Kinds.SlotNames, ref slotNames);
        }

-        public static bool HasKeyValues(this SchemaShape.Column col)
+        public static bool ShouldAddSlotNames(this SchemaShape.Column col)


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

NeedsSlotNames? Having a modal verb here feels odd to me. #Resolved

eerhardt · 2019-03-04T21:22:06Z

src/Microsoft.ML.Core/Data/AnnotationUtils.cs

        {
            return col.Annotations.TryFindColumn(Kinds.KeyValues, out var metaCol)
                && metaCol.Kind == SchemaShape.Column.VectorKind.Vector
-                && metaCol.ItemType is TextDataViewType;
+                && metaCol.ItemType is TextDataViewType; ;


double ; #Resolved

eerhardt · 2019-03-04T21:31:01Z

test/Microsoft.ML.Tests/PermutationFeatureImportanceTests.cs

+            if (task == TaskType.BinaryClassification)
+                return pipeline.Append(ML.Transforms.Conversion.ConvertType("Label", outputKind: DataKind.Boolean))
+                    .Fit(srcDV).Transform(srcDV);
+            else


else if should be on one line. #Resolved

Ivanidzo4ka · 2019-03-04T22:14:59Z

namespace Microsoft.ML.Trainers.FastTree

Good question.
Main reason for this PR is to have consistency and support only one type of label.
Boolean feels natural for binary classification, but I can see benefits of floats for binary classification.
How useful that conversion is?
For Gam we just do this:
boolArray[doc] = targets[doc] > 0;
Which is same thing as ConvertFromR4ToBool.
Not sure I know where to look for FastTree.

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

Refers to: src/Microsoft.ML.FastTree/FastTreeRanking.cs:40 in 5707e17. [](commit_id = 5707e17, deletion_comment = False)

rogancarr · 2019-03-04T22:57:10Z

namespace Microsoft.ML.Trainers.FastTree

In FastTree, we do this crazy thing:

private IEnumerable<bool> GetClassificationLabelsFromRatings(Dataset set)
{
    // REVIEW: Historically FastTree has this test as >= 1. TLC however   
    // generally uses > 0. Consider changing FastTree to be consistent.
    return set.Ratings.Select(x => x >= 1);
}

I would support dropping support for ints & floats & doubles here because we don't use any of the information and we do confusing casts.

In reply to: 469443560 [](ancestors = 469443560,469402531)

Refers to: src/Microsoft.ML.FastTree/FastTreeRanking.cs:40 in 5707e17. [](commit_id = 5707e17, deletion_comment = False)

rogancarr · 2019-03-04T23:00:23Z

namespace Microsoft.ML.Trainers.FastTree

Note that one thing we don't do is allow probability-as-labels for calibrated trainers. That is, there are times when I may want to specify that I believe that the correct probability out is 0.7 and not 1.0 for an example.

This is the only case where we'd want to allow floats as inputs.

In reply to: 469456048 [](ancestors = 469456048,469443560,469402531)

Refers to: src/Microsoft.ML.FastTree/FastTreeRanking.cs:40 in 5707e17. [](commit_id = 5707e17, deletion_comment = False)

sfilipi · 2019-03-06T23:03:00Z

test/Microsoft.ML.Tests/PermutationFeatureImportanceTests.cs

@@ -40,14 +40,14 @@ public void TestPfiRegressionOnDenseFeatures()
            // X4Rand: 3

            // For the following metrics lower is better, so maximum delta means more important feature, and vice versa
-            Assert.Equal(3, MinDeltaIndex(pfi, m => m.MeanAbsoluteError.Mean));


[](start = 0, length = 6)

one too many #Resolved

i mean new lines

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

sfilipi · 2019-03-06T23:05:18Z

test/Microsoft.ML.Tests/Scenarios/Api/TestApi.cs

@@ -294,7 +294,7 @@ private List<BreastCancerExample> ReadBreastCancerExamples()
        public void TestTrainTestSplit()
        {
            var mlContext = new MLContext(0);
-            
+
            var dataPath = GetDataPath("adult.tiny.with-schema.txt");


one #Resolved

one what?

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

one empty line, or did you mean to have two?

In reply to: 263510301 [](ancestors = 263510301,263175595)

It's actually codeflow specific. it show two new lines if you remove leading spaces from it

In reply to: 263685676 [](ancestors = 263685676,263510301,263175595)

wschin · 2019-03-06T23:28:39Z

test/BaselineOutput/Common/FeatureContribution/FastForestBinary.tsv

-#@   col=FeatureContributions:R4:25-30
-#@   col=FeatureContributions:R4:31-36
-#@   col=FeatureContributions:R4:37-42
+#@   col=Label:BL:7


Why do we need two labels columns? #ByDesign

We restrict binary classification to work only with boolean columns.
It's easier to add convert than change shared code which generate R4 labels for different classes.

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

wschin · 2019-03-06T23:30:54Z

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

@@ -1523,20 +1523,6 @@ private protected SdcaBinaryTrainerBase(IHostEnvironment env, BinaryOptionsBase

        private protected abstract SchemaShape.Column[] ComputeSdcaBinaryClassifierSchemaShape();

-        private protected override void CheckLabelCompatible(SchemaShape.Column labelCol)


CheckLabelCompatible [](start = 40, length = 20)

Can label be text? If no, we still a check. #Resolved

As you can see this is override method. By removing this one, I would force system to fallback to default behavior. Which is - to check it's a boolean column.

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

wschin · 2019-03-07T00:10:45Z

src/Microsoft.ML.Data/Data/Conversion.cs

@@ -899,6 +909,18 @@ public void Convert(in BL src, ref SB dst)
        public void Convert(in DT src, ref SB dst) { ClearDst(ref dst); dst.AppendFormat("{0:o}", src); }
        public void Convert(in DZ src, ref SB dst) { ClearDst(ref dst); dst.AppendFormat("{0:o}", src); }
        #endregion ToStringBuilder
+        #region ToBL
+        public void Convert(in R8 src, ref BL dst) => dst = src > 0 ? true : false;


Suggested change

public void Convert(in R8 src, ref BL dst) => dst = src > 0 ? true : false;

public void Convert(in R8 src, ref BL dst) => dst = src > 0.5 ? true : false;

Rounding to the nearest number looks more reasonable. #Closed

I'm not sure about that.

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

I think 0.00000001 looks more closer to 0?

In reply to: 263196830 [](ancestors = 263196830,263190893)

wschin · 2019-03-07T00:10:58Z

src/Microsoft.ML.Data/Data/Conversion.cs

@@ -899,6 +909,18 @@ public void Convert(in BL src, ref SB dst)
        public void Convert(in DT src, ref SB dst) { ClearDst(ref dst); dst.AppendFormat("{0:o}", src); }
        public void Convert(in DZ src, ref SB dst) { ClearDst(ref dst); dst.AppendFormat("{0:o}", src); }
        #endregion ToStringBuilder
+        #region ToBL
+        public void Convert(in R8 src, ref BL dst) => dst = src > 0 ? true : false;
+        public void Convert(in R4 src, ref BL dst) => dst = src > 0 ? true : false;


Suggested change

public void Convert(in R4 src, ref BL dst) => dst = src > 0 ? true : false;

public void Convert(in R4 src, ref BL dst) => dst = src > 0.5 ? true : false;

``` #Closed

wschin · 2019-03-07T00:13:43Z

test/BaselineOutput/Common/FeatureContribution/AveragePerceptronBinary.tsv

-#@   col=FeatureContributions:R4:25-30
-#@   col=FeatureContributions:R4:31-36
-#@   col=FeatureContributions:R4:37-42
+#@   col=Label:BL:7


Any reason to have two label columns? #ByDesign

ByDesign — response on a different thread.

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

wschin · 2019-03-07T00:15:30Z

test/Microsoft.ML.Core.Tests/UnitTests/TestEntryPoints.cs

+            TestEntryPointRoutine("iris.txt", "Trainers.EnsembleBinaryClassifier", xfNames:
+                new[] {
+                    "Transforms.ColumnTypeConverter",
+


Extra line? #Resolved

wschin · 2019-03-07T00:15:44Z

test/Microsoft.ML.Core.Tests/UnitTests/TestEntryPoints.cs

+                       {{
+                         {string.Format(xfTemplate, xfNames[i], i + 1, xfArgs[i], i + 2)}
+                       }},";
+


Extra line? #Resolved

wschin · 2019-03-07T00:17:07Z

test/Microsoft.ML.TestFramework/Datasets.cs

-            fileSeparator = '\t'
+            fileSeparator = '\t',
+            mamlExtraSettings = new[] { "xf=Term{col=Label}" }
+


Extra line? Can auto-formatting handle it? #Resolved

wschin · 2019-03-07T00:18:19Z

test/Microsoft.ML.Tests/FeatureContributionTests.cs

@@ -1,344 +0,0 @@
-// Licensed to the .NET Foundation under one or more agreements.


Is the removal of this file intended? #Resolved

I have no idea why this is happening....

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

wschin · 2019-03-07T00:20:37Z

Do we have any test to ensure that wrong label type incurs exception?

rogancarr · 2019-03-07T19:40:11Z

test/Microsoft.ML.Tests/FeatureContributionTests.cs

+        /// 2nd slot of xBuff has the least importance: Evaluation metrics do not change a lot when this slot is permuted.
+        /// x3 has the biggest importance.
+        /// </summary>
+        private IDataView GetSparseDataset(TaskType task = TaskType.Regression, int numberOfInstances = 1000)


Can we put these into a common static library where it can be accessed from all the places that use it, rather than copy it here?

sfilipi

rogancarr · 2019-03-08T22:12:29Z

Side note: This description is pretty vague ! :)

rogancarr · 2019-03-08T22:14:11Z

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

@@ -145,7 +145,14 @@ private protected virtual void CheckLabelCompatible(SchemaShape.Column labelCol)
            IDataView validationSet = null, IPredictor initPredictor = null)
        {
            var trainRoleMapped = MakeRoles(trainSet);
-            var validRoleMapped = validationSet == null ? null : MakeRoles(validationSet);
+            CheckInputSchema(SchemaShape.Create(trainSet.Schema));


CheckInputSchema [](start = 12, length = 16)

Nit: Ordering for train and valid in Make and Check are different. I'd prefer to have the same sequence. #Resolved

rogancarr · 2019-03-08T22:15:34Z

src/Microsoft.ML.StandardLearners/Standard/MultiClass/MetaMulticlassTrainer.cs

@@ -99,19 +98,14 @@ private protected IDataView MapLabelsCore<T>(DataViewType type, InPredicate<T> e
            Host.Assert(data.Schema.Label.HasValue);

            var lab = data.Schema.Label.Value;


lab [](start = 16, length = 3)

Nit: This hurts my eyes. #Resolved

rogancarr

A key fix!

Just some nits but you are ready to go.

👨‍🚀 🚀 🌕 🌎

Ivan Matantsev added 8 commits February 28, 2019 15:16

Let's see how many will fail

62c50a3

Update some tests

1631815

Merge with master

1ae2d9a

revert iris

1620d74

fix more tests

f90311b

Merge branch 'master' into Ivanidze/OneTypePolicyInTrainers

9f93371

fix entrypoint tests

395bc18

update baseline

9dff7be

Ivanidzo4ka changed the title ~~[WIP] One type label policy in trainers~~ One type label policy in trainers Mar 4, 2019

Ivanidzo4ka mentioned this pull request Mar 4, 2019

Train binary classification with text label #2826

Closed

add key transform to benchmark

5707e17

Ivanidzo4ka requested review from TomFinley, yaeldekel, sfilipi, wschin and rogancarr March 4, 2019 18:44

rogancarr reviewed Mar 4, 2019

View reviewed changes

eerhardt reviewed Mar 4, 2019

View reviewed changes

small cleanup

538c7c1

Ivanidzo4ka mentioned this pull request Mar 6, 2019

Different behavior when calling Fit() on a transformer chain and on an IDataLoaderEstimator #2866

Closed

merge with master

909ba4e

update

84dc7ce

sfilipi reviewed Mar 6, 2019

View reviewed changes

wschin reviewed Mar 6, 2019

View reviewed changes

wschin reviewed Mar 7, 2019

View reviewed changes

Ivan Matantsev added 4 commits March 7, 2019 09:24

update

fe64174

another update

a08a0ac

Standard conversion to bool

be2afa2

after effects of deleted files

55565d2

rogancarr reviewed Mar 7, 2019

View reviewed changes

another thing

aeb7338

sfilipi approved these changes Mar 8, 2019

View reviewed changes

rogancarr reviewed Mar 8, 2019

View reviewed changes

rogancarr approved these changes Mar 8, 2019

View reviewed changes

Ivan Matantsev added 2 commits March 11, 2019 12:43

address some comments

c173249

merge with master

7d154e5

Ivanidzo4ka merged commit acc4ac0 into dotnet:master Mar 12, 2019

eerhardt mentioned this pull request Mar 28, 2019

Prior binary trainer requires float label column, but all other binary trainers require bool #3119

Closed

eerhardt mentioned this pull request Sep 27, 2019

Jupyter + ML.NET | DataFrame vs Dataview #4252

Closed

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

		@@ -1523,20 +1523,6 @@ private protected SdcaBinaryTrainerBase(IHostEnvironment env, BinaryOptionsBase

		private protected abstract SchemaShape.Column[] ComputeSdcaBinaryClassifierSchemaShape();

		private protected override void CheckLabelCompatible(SchemaShape.Column labelCol)

	public void Convert(in R8 src, ref BL dst) => dst = src > 0 ? true : false;
	public void Convert(in R8 src, ref BL dst) => dst = src > 0.5 ? true : false;

	public void Convert(in R4 src, ref BL dst) => dst = src > 0 ? true : false;
	public void Convert(in R4 src, ref BL dst) => dst = src > 0.5 ? true : false;
	``` #Closed

		@@ -1,344 +0,0 @@
		// Licensed to the .NET Foundation under one or more agreements.

		@@ -99,19 +98,14 @@ private protected IDataView MapLabelsCore<T>(DataViewType type, InPredicate<T> e
		Host.Assert(data.Schema.Label.HasValue);

		var lab = data.Schema.Label.Value;

One type label policy in trainers #2804

One type label policy in trainers #2804

Conversation

Ivanidzo4ka commented Feb 28, 2019 • edited Loading

codecov bot commented Mar 2, 2019 • edited Loading

Codecov Report

rogancarr commented Mar 4, 2019

rogancarr Mar 4, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

rogancarr Mar 4, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rogancarr Mar 4, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

eerhardt Mar 4, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

eerhardt Mar 4, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Ivanidzo4ka commented Mar 4, 2019

rogancarr commented Mar 4, 2019 • edited Loading

rogancarr commented Mar 4, 2019 • edited Loading

sfilipi Mar 6, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Mar 6, 2019 • edited by Ivanidzo4ka 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

wschin Mar 6, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wschin Mar 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wschin Mar 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wschin Mar 7, 2019 • edited Loading

Choose a reason for hiding this comment

wschin Mar 7, 2019 • edited by rogancarr Loading

Choose a reason for hiding this comment

rogancarr Mar 8, 2019 • edited Loading

Choose a reason for hiding this comment

wschin Mar 7, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

wschin Mar 7, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

wschin Mar 7, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

wschin Mar 7, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wschin commented Mar 7, 2019

Choose a reason for hiding this comment

sfilipi left a comment

Choose a reason for hiding this comment

rogancarr commented Mar 8, 2019

rogancarr Mar 8, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

rogancarr Mar 8, 2019 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

rogancarr left a comment • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka commented Feb 28, 2019 •

edited

Loading

codecov bot commented Mar 2, 2019 •

edited

Loading

rogancarr Mar 4, 2019 •

edited by Ivanidzo4ka

Loading

rogancarr Mar 4, 2019 •

edited by Ivanidzo4ka

Loading

rogancarr Mar 4, 2019 •

edited by Ivanidzo4ka

Loading

eerhardt Mar 4, 2019 •

edited by Ivanidzo4ka

Loading

eerhardt Mar 4, 2019 •

edited by Ivanidzo4ka

Loading

rogancarr commented Mar 4, 2019 •

edited

Loading

rogancarr commented Mar 4, 2019 •

edited

Loading

sfilipi Mar 6, 2019 •

edited by Ivanidzo4ka

Loading

sfilipi Mar 6, 2019 •

edited by Ivanidzo4ka

Loading

wschin Mar 6, 2019 •

edited by Ivanidzo4ka

Loading

wschin Mar 6, 2019 •

edited

Loading

wschin Mar 7, 2019 •

edited

Loading

wschin Mar 7, 2019 •

edited

Loading

wschin Mar 7, 2019 •

edited by rogancarr

Loading

rogancarr Mar 8, 2019 •

edited

Loading

wschin Mar 7, 2019 •

edited by Ivanidzo4ka

Loading

wschin Mar 7, 2019 •

edited by Ivanidzo4ka

Loading

wschin Mar 7, 2019 •

edited by Ivanidzo4ka

Loading

wschin Mar 7, 2019 •

edited by Ivanidzo4ka

Loading

rogancarr Mar 8, 2019 •

edited by Ivanidzo4ka

Loading

rogancarr Mar 8, 2019 •

edited by Ivanidzo4ka

Loading

rogancarr left a comment •

edited

Loading