Role mapped improvements #496

TomFinley · 2018-07-05T15:17:32Z

Fixes #445.

Generally, favors creating RoleMappedSchema/RoleMappedData by actual constructors, since that's how objects are generally created.
Concentrated the actual globally useful "conveniences" inside the classes themselves, rather than in completely undiscoverable Utils classes.
Got rid of the Create and CreateOpt idiom that required that we declare every creation method twice in favor of a simpler bool parameter on the constructors.
Added documentation.

Ivanidzo4ka · 2018-07-05T17:22:12Z

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

@@ -312,7 +312,6 @@ public static bool HasSlotNames(this ISchema schema, int col, int vectorSize)
        public static void GetSlotNames(RoleMappedSchema schema, RoleMappedSchema.ColumnRole role, int vectorSize, ref VBuffer<DvText> slotNames)
        {
            Contracts.CheckValueOrNull(schema);
-            Contracts.CheckValue(role.Value, nameof(role));


Contracts.CheckValue(role.Value, nameof(role)); [](start = 12, length = 47)

you removing it because ColumnRole has only one constuctor in which we invoke Contracts.CheckNonEmpty, so Value always gonna have Value, right? #Closed

Right. In the issue #445 when I said "Also a fair amount of code exists in the class to detect conditions that cannot possibly happen." this is partially what I am referring to. Indeed I think it's the only real reason to have a separate struct instead of just using string directly, so it's a little weird that we spent so much effort checking and rechecking. #Closed

Ivanidzo4ka · 2018-07-05T17:28:42Z

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

+        /// <param name="schema">The schema over which roles are defined</param>
+        /// <param name="roles">The column role to column name mappings</param>
+        /// <param name="opt">Whether to consider the column names specified "optional" or not. If <c>false</c> then any non-empty
+        /// values for the column names that does not appear in <paramref name="schema"/> will result iin an exception being thrown,


iin [](start = 102, length = 3)

iin #Closed

Ivanidzo4ka · 2018-07-05T17:28:47Z

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

-        public static RoleMappedSchema Create(ISchema schema)
+        /// <param name="schema">The schema over which roles are defined</param>
+        /// <param name="opt">Whether to consider the column names specified "optional" or not. If <c>false</c> then any non-empty
+        /// values for the column names that does not appear in <paramref name="schema"/> will result iin an exception being thrown,


iin [](start = 102, length = 3)

iin #Closed

Ivanidzo4ka · 2018-07-05T17:34:48Z

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

@@ -363,45 +380,82 @@ public ColumnInfo GetUniqueColumn(ColumnRole role)
        }

        /// <summary>
-        /// Creates a RoleMappedSchema from the given schema with no column role assignments.
+        /// Constructor from the given schema and role/column-name pairs.


role/column-name [](start = 50, length = 16)

I can read it as role | column-name pairs or
(role or column) | name pair.
Which probably show my lack of English, but still can it be rephrased? #Closed

Hmmm OK. I guess I could omit names since columns are always identified by name, that might make clear I'm not talking about the strings used to construct the roles in the first place. #Closed

Ivanidzo4ka · 2018-07-05T17:38:41Z

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

-            if (role.Value != null && _map.TryGetValue(role.Value, out list))
-                return list;
-            return null;
+            return _map.TryGetValue(role.Value, out var list) ? list : null;


return [](start = 12, length = 6)

it look strange to see all methods above get converted from return to => and this one still use return. #Closed

Ivanidzo4ka · 2018-07-05T17:43:55Z

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

@@ -4,6 +4,7 @@

 using System;
 using System.Collections.Generic;
+using System.Linq;


using System.Linq [](start = 0, length = 17)

My VS highlight gray using System and using System.Linq for me and suggest to remove them as unused. #Closed

Ivanidzo4ka · 2018-07-05T17:45:07Z

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

@@ -36,7 +37,7 @@ public static ColumnInfo CreateFromName(ISchema schema, string name, string desc
        {
            ColumnInfo colInfo;


ColumnInfo colInfo; [](start = 12, length = 19)

can be moved into method call
out ColumnInfo colInfo #Closed

Ivanidzo4ka · 2018-07-05T17:46:07Z

        int index;

can be moved to method call.
(I wouldn't write this comment generally, but you done this thing in all other places in this file, so they look like outliers.) #Closed

Refers to: src/Microsoft.ML.Core/Data/RoleMappedSchema.cs:55 in 158b653. [](commit_id = 158b653, deletion_comment = False)

Ivanidzo4ka · 2018-07-05T17:51:16Z

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

@@ -98,68 +112,99 @@ public sealed class RoleMappedSchema
        private const string NameString = "Name";
        private const string FeatureContributionsString = "FeatureContributions";

+        /// <summary>
+        /// Instances of this are the keys of a <see cref="RoleMappedSchema"/>. This class also some important commonly used


some [](start = 95, length = 6)

this phrasing looks weird for me, but what do I know #Closed

You know lots Ivan!

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

Ivanidzo4ka · 2018-07-05T17:57:05Z

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

+            /// <summary>
+            /// Role for sample weights. Commonly used to point to a number to make trainers give more weight
+            /// to a particular example.
+            /// </summary>
            public static ColumnRole Weight => WeightString;
            public static ColumnRole Name => NameString;


public static ColumnRole Name => NameString; [](start = 12, length = 44)

considering you make reference to this column (see line 205) it's probably make sense to add description for this as well. #Closed

Yes... also I think FeatureContributions should be done away with, I'm not sure why it is here. Maybe someone thought once upon a time that for a column role to be used, it has to be here.

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

TomFinley · 2018-07-05T18:07:30Z

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

+            // REVIEW: Does this really belong here?
+            /// <summary>
+            /// Role for feature contributions. Useful for specific diagnostic functionality.
+            /// </summary>
            public static ColumnRole FeatureContributions => FeatureContributionsString;


FeatureContributions [](start = 37, length = 20)

I don't really feel like this actually belongs here... certainly in ML.NET there are no usages, and the usages elsewhere are heavily localized so putting a custom role in the basic class is sort of odd... it would be better to have the score-roles.

Ivanidzo4ka · 2018-07-05T18:59:01Z

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

@@ -98,68 +108,109 @@ public sealed class RoleMappedSchema
        private const string NameString = "Name";
        private const string FeatureContributionsString = "FeatureContributions";

+        /// <summary>
+        /// Instances of this are the keys of a <see cref="RoleMappedSchema"/>. This class also some holds important


some holds [](start = 96, length = 10)

are you sure it's correct order? #Closed

Um no. Sorry. :) Me speak english pretty one day.

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

Ivanidzo4ka · 2018-07-05T19:02:46Z

src/Microsoft.ML.Data/Commands/ScoreCommand.cs

-            var schema = TrainUtils.CreateRoleMappedSchemaOpt(input.Schema, featureColName, groupColName, customColumns);
-            ISchemaBoundMapper mapper;
-            var sc = GetScorerComponentAndMapper(predictor, scorer, schema, env, out mapper);
+            var schema = new RoleMappedSchema(input.Schema, label: null, feature: featureColName, group: groupColName, opt: true);


I think you lost customColumns here.
at least we used to pass them. #Closed

Whoops

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

Ivanidzo4ka · 2018-07-05T19:04:58Z

src/Microsoft.ML.Data/Depricated/Instances/HeaderSchema.cs

@@ -379,8 +377,9 @@ public Dense(int count, string[] names)
                if (size > 0)
                    Array.Copy(names, _names, size);

-                _schema = RoleMappedSchema.Create(new FeatureNameCollectionSchema(this),
-                    RoleMappedSchema.CreatePair(RoleMappedSchema.ColumnRole.Feature, RoleMappedSchema.ColumnRole.Feature.Value));
+                // REVIEW: This seems wrong. The default feature column name is "Features" yet the role is named "Feature".


// REVIEW: This seems wrong. The default feature column name is "Features" yet the role is named "Feature". [](start = 16, length = 107)

Should we move this review to RoleMappedSchema file? #ByDesign

That's not really waht I mean. I think it's fine to have the role be "feature", but the column name be "features." What I meant was, this very specific usage of it in this file.

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

Ivanidzo4ka · 2018-07-05T19:08:23Z

src/Microsoft.ML.Data/Evaluators/MamlEvaluator.cs

@@ -127,11 +127,11 @@ protected MamlEvaluatorBase(ArgumentsBase args, IHostEnvironment env, string sco

            // Get the label column information.
            string lab = EvaluateUtils.GetColName(LabelCol, schema.Label, DefaultColumnNames.Label);


string lab [](start = 12, length = 10)

can we change it to var label?
it just looks odd to have lab and then few lines later weight. #Closed

Mind if I go the other way? I like to be explicit with my types (in this case not very obvious) unless it's something longer, or the type is immediately obvious to a reader. (Except int... don't get people that aren't explicit about int.)

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

As you can probably see from my comment, I tend to prefer var, but i don't have problem with explicit types, it was more about shortening label to lab, which for some reason bugs me enormously.

In reply to: 200460563 [](ancestors = 200460563,200458796)

Oh I see...

In reply to: 200462127 [](ancestors = 200462127,200460563,200458796)

Ivanidzo4ka · 2018-07-05T19:12:13Z

src/Microsoft.ML.Ensemble/OutputCombiners/BaseStacking.cs

@@ -185,7 +185,7 @@ public void Train(List<FeatureSubsetModel<IPredictorProducing<TOutput>>> models,
                bldr.AddColumn("Features", NumberType.Float, features);

                var view = bldr.GetDataView();
-                var rmd = RoleMappedData.Create(view, ColumnRole.Label.Bind("Label"), ColumnRole.Feature.Bind("Features"));
+                var rmd = new RoleMappedData(view, "Label", "Features");


"Label", "Features" [](start = 51, length = 19)

can we replace it with DefaultColumnNames.Label, DefaultColumnNames.Features
same for line 184 and line 185 #Closed

Ah good one. Also SmacSweeper...

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

Ivanidzo4ka · 2018-07-05T19:13:27Z

src/Microsoft.ML.FastTree/GamTrainer.cs

-                        _dataForEvaluator = RoleMappedData.Create(builder.GetDataView(), RoleMappedSchema.ColumnRole.Label.Bind("Label"),
-                            RoleMappedSchema.CreatePair(MetadataUtils.Const.ScoreValueKind.Score, "Score"));
+                        _dataForEvaluator = new RoleMappedData(builder.GetDataView(), opt: false, RoleMappedSchema.ColumnRole.Label.Bind("Label"),
+                            new RoleMappedSchema.ColumnRole(MetadataUtils.Const.ScoreValueKind.Score).Bind("Score"));


DefaultColumnNames.Label, DefaultColumnNames.Score ? same for few lines above #Closed

Ivanidzo4ka · 2018-07-05T19:14:03Z

src/Microsoft.ML.KMeansClustering/KMeansPlusPlusTrainer.cs

@@ -902,7 +902,7 @@ public bool CanWeightComputationBeAvoided(Float instanceDistanceToBestOldCluster
                    arrDv.AddColumn("Features", PrimitiveType.FromKind(DataKind.R4), clusters);
                    arrDv.AddColumn("Weights", PrimitiveType.FromKind(DataKind.R4), totalWeights);
                    var subDataViewCursorFactory = new FeatureFloatVectorCursor.Factory(
-                        TrainUtils.CreateExamples(arrDv.GetDataView(), null, "Features", weight: "Weights"), CursOpt.Weight | CursOpt.Features);
+                        new RoleMappedData(arrDv.GetDataView(), null, "Features", weight: "Weights"), CursOpt.Weight | CursOpt.Features);


Features [](start = 71, length = 8)

DefaultColumnNames? #Closed

Yup. And I see here instead of DefaultColumnNames.Weight which is "Weight" we instead used "Weights", which is not great. (Just inconsistent, probably doesn't change things.)

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

Ivanidzo4ka · 2018-07-05T19:15:21Z

        dvBuilder.AddColumn("Features", NumberType.Float, features);

DefaultColumnNames? #Closed

Refers to: src/Microsoft.ML.Sweeper/Algorithms/SmacSweeper.cs:130 in b405fe0. [](commit_id = b405fe0, deletion_comment = False)

Ivanidzo4ka · 2018-07-05T19:15:44Z

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

-            var rmd = RoleMappedData.Create(splitOutput.TrainData[0],
-                RoleMappedSchema.CreatePair(RoleMappedSchema.ColumnRole.Feature, "Features"),
-                RoleMappedSchema.CreatePair(RoleMappedSchema.ColumnRole.Label, "Label"));
+            var rmd = new RoleMappedData(splitOutput.TrainData[0], "Label", "Features");


"Features" [](start = 76, length = 10)

DefaultColumnNames? #Closed

I'm a little less excited about this for unit tests.

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

it's totally up to you, and probably outside of scope for this PR :)

In reply to: 200461719 [](ancestors = 200461719,200461236)

I don't mind that. I think I ought to clean up the actual production code to conform to best practices as you suggest. The tests though -- the goal of those is a little different. In the actual code some obscurity to be assured of consistency is good, but I'm not sure obscurity works for tests.

In reply to: 200462334 [](ancestors = 200462334,200461719,200461236)

…re, other improvements.

Ivanidzo4ka

zeahmed · 2018-07-05T21:36:59Z

src/Microsoft.ML.KMeansClustering/KMeansPlusPlusTrainer.cs

                    var subDataViewCursorFactory = new FeatureFloatVectorCursor.Factory(
-                        TrainUtils.CreateExamples(arrDv.GetDataView(), null, "Features", weight: "Weights"), CursOpt.Weight | CursOpt.Features);
+                        new RoleMappedData(arrDv.GetDataView(), null, DefaultColumnNames.Features,weight: DefaultColumnNames.Weight), CursOpt.Weight | CursOpt.Features);


formatting: space before weight parameter.

Thanks @zeahmed !

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

zeahmed

Looks good to me. Thanks @TomFinley for bring these conveniences around core API.

TomFinley · 2018-07-05T23:17:57Z

@dotnet-bot test Windows_NT Debug

Ivanidzo4ka

* RoleMappedSchema/Data change to use constructors * Nuke all create methods, pointless "no-roles" constructor. * Nuke TrainUtils.CreateExamples/CreateExamplesOpt * Opportunistically improve code quality and reporting of Kmeans++

TomFinley requested review from Ivanidzo4ka, shauheen, yaeldekel and zeahmed July 5, 2018 15:17

Tom Finley and others added 7 commits July 5, 2018 08:46

RoleMappedSchema/Data change to use constructors

a30d9d2

Obsolescence of general RoleMappedSchema utils from TrainUtils

fdc5445

Improve documentation and usage

7a027d3

little things on ScoreCommand.cs

4273205

Nuke all create methods, pointless "no-roles" constructor.

b2ba7d8

One more

c81a7fe

Nuke TrainUtils.CreateExamples/CreateExamplesOpt

158b653

TomFinley force-pushed the tfinley/RoleMap branch from d618134 to 158b653 Compare July 5, 2018 15:46

Ivanidzo4ka reviewed Jul 5, 2018

View reviewed changes

TomFinley added 2 commits July 5, 2018 11:00

Ivan comments

6af4a86

Overlooked Ivan comments

b405fe0

TomFinley commented Jul 5, 2018

View reviewed changes

Ivanidzo4ka reviewed Jul 5, 2018

View reviewed changes

TomFinley added 2 commits July 5, 2018 12:24

More Ivan comments. Default column names, custom role mappings in sco…

4d513b3

…re, other improvements.

lab => label

fd674df

Ivanidzo4ka approved these changes Jul 5, 2018

View reviewed changes

zeahmed reviewed Jul 5, 2018

View reviewed changes

Weight spacing issue

19f2467

zeahmed approved these changes Jul 5, 2018

View reviewed changes

Opportunistically improve code quality and reporting of Kmeans++

50fa086

Ivanidzo4ka approved these changes Jul 5, 2018

View reviewed changes

TomFinley merged commit f85e722 into dotnet:master Jul 5, 2018

TomFinley deleted the tfinley/RoleMap branch July 5, 2018 23:42

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

		@@ -36,7 +37,7 @@ public static ColumnInfo CreateFromName(ISchema schema, string name, string desc
		{
		ColumnInfo colInfo;

		@@ -127,11 +127,11 @@ protected MamlEvaluatorBase(ArgumentsBase args, IHostEnvironment env, string sco

		// Get the label column information.
		string lab = EvaluateUtils.GetColName(LabelCol, schema.Label, DefaultColumnNames.Label);

Role mapped improvements #496

Role mapped improvements #496

Uh oh!

Conversation

TomFinley commented Jul 5, 2018

Uh oh!

Ivanidzo4ka Jul 5, 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!

TomFinley Jul 5, 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!

Ivanidzo4ka Jul 5, 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!

Ivanidzo4ka Jul 5, 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!

Ivanidzo4ka Jul 5, 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!

TomFinley Jul 5, 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!

Ivanidzo4ka Jul 5, 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!

Ivanidzo4ka Jul 5, 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!

Ivanidzo4ka Jul 5, 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!

Ivanidzo4ka commented Jul 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ivanidzo4ka Jul 5, 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!

Ivanidzo4ka Jul 5, 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!

Ivanidzo4ka Jul 5, 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!

Ivanidzo4ka Jul 5, 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!

Ivanidzo4ka Jul 5, 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!

TomFinley Jul 5, 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!

Ivanidzo4ka Jul 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Ivanidzo4ka Jul 5, 2018 •

edited

Loading

TomFinley Jul 5, 2018 •

edited

Loading

Ivanidzo4ka Jul 5, 2018 •

edited

Loading

Ivanidzo4ka Jul 5, 2018 •

edited

Loading

Ivanidzo4ka Jul 5, 2018 •

edited

Loading

TomFinley Jul 5, 2018 •

edited

Loading

Ivanidzo4ka Jul 5, 2018 •

edited

Loading

Ivanidzo4ka Jul 5, 2018 •

edited

Loading

Ivanidzo4ka Jul 5, 2018 •

edited

Loading

Ivanidzo4ka commented Jul 5, 2018 •

edited

Loading

Ivanidzo4ka Jul 5, 2018 •

edited

Loading

Ivanidzo4ka Jul 5, 2018 •

edited

Loading

Ivanidzo4ka Jul 5, 2018 •

edited

Loading

Ivanidzo4ka Jul 5, 2018 •

edited

Loading

Ivanidzo4ka Jul 5, 2018 •

edited

Loading

TomFinley Jul 5, 2018 •

edited

Loading

Ivanidzo4ka Jul 5, 2018 •

edited

Loading

Ivanidzo4ka Jul 5, 2018 •

edited

Loading

Ivanidzo4ka Jul 5, 2018 •

edited

Loading

Ivanidzo4ka Jul 5, 2018 •

edited

Loading

Ivanidzo4ka commented Jul 5, 2018 •

edited

Loading

Ivanidzo4ka Jul 5, 2018 •

edited

Loading