Input output swap by sfilipi · Pull Request #2239 · dotnet/machinelearning

sfilipi · 2019-01-25T01:07:35Z

Fixes issue #2064

1- Change the order of the parameters from inputColumn, outputColumn to outputColumnName, sourceColumnName.

2 - Changing the "source" parameter name and field in the Columninfo classes, to be "sourceColumnName", as suggested.

3- Changing the "name" parameter to "outputColumnName" in the:

estimator extension APIs
estimator ctors
column pairs expressed through tuples, because in context it reads better than name.

Note: in the columnInfo classes i left it to "name" because "outputColumnName" makes no sense.

4 - Nit on standardizing the XML comments.
5 - Arranging the order of the parameters to be: outputColumnName, required parameters, nullable sourceColumnName.
6 - fixed some bugs i bumped into.

codecov · 2019-01-25T01:56:36Z

Codecov Report

Merging #2239 into master will increase coverage by 5.7%.
The diff coverage is 77.8%.

@@            Coverage Diff             @@
##           master    #2239      +/-   ##
==========================================
+ Coverage   65.43%   71.13%    +5.7%     
==========================================
  Files         766      779      +13     
  Lines      133371   140271    +6900     
  Branches    15669    16046     +377     
==========================================
+ Hits        87269    99785   +12516     
+ Misses      41889    36034    -5855     
- Partials     4213     4452     +239

Flag	Coverage Δ
#Debug	`71.13% <77.8%> (+5.7%)`	⬆️
#production	`67.56% <70%> (+5.27%)`	⬆️
#test	`85.05% <95.14%> (+3.52%)`	⬆️

sfilipi · 2019-01-25T06:51:58Z

            /// </summary>
            [BestFriend]
-            internal bool IsCompatibleWith(Column inputColumn)
+            internal bool IsCompatibleWith(Column source)


Column source) [](start = 43, length = 14)

unnecessary change.. #Resolved

sfilipi · 2019-01-25T06:55:57Z

            for (int i = 0; i < columns.Length; i++)
            {
-                var col = inputSchema.GetColumnOrNull(columns[i].Input);
+                var col = inputSchema.GetColumnOrNull(columns[i].Source);


Source [](start = 65, length = 6)

SourceColumnName #Resolved

sfilipi · 2019-01-25T07:03:07Z

-            public readonly string Input;
-            public readonly string Output;
+            public readonly string Name;
+            public readonly string Source;


Source [](start = 35, length = 6)

SourceColumnName #Resolved

sfilipi · 2019-01-25T07:04:04Z

-            string output = null, ImagePixelExtractorTransformer.ColorBits colors = ImagePixelExtractorTransformer.Defaults.Colors,
+        public ImagePixelExtractingEstimator(IHostEnvironment env,
+            string name,
+            string source = null,


source [](start = 19, length = 6)

sourceColumnName #Resolved

sfilipi · 2019-01-25T07:04:20Z

-        public ImagePixelExtractingEstimator(IHostEnvironment env, string input,
-            string output = null, ImagePixelExtractorTransformer.ColorBits colors = ImagePixelExtractorTransformer.Defaults.Colors,
+        public ImagePixelExtractingEstimator(IHostEnvironment env,
+            string name,


name [](start = 19, length = 4)

outputColumnName #Resolved

sfilipi · 2019-01-25T07:05:43Z

-            public readonly string Input;
-            public readonly string Output;
+            public readonly string Name;
+            public readonly string Source;


Source [](start = 35, length = 6)

SourceColumName #Resolved

sfilipi · 2019-01-25T07:08:53Z

-        public DnnImageFeaturizerEstimator(IHostEnvironment env, Func<DnnImageFeaturizerInput, EstimatorChain<ColumnCopyingTransformer>> modelFactory, string inputColumn, string outputColumn)
+        /// <param name="outputColumnName">Name of the column resulting from the transformation of <paramref name="sourceColumnName"/>.</param>
+        /// <param name="sourceColumnName">Name of column to transform. If set to <see langword="null"/>, the value of the <paramref name="outputColumnName"/> will be used as source.</param>
+        public DnnImageFeaturizerEstimator(IHostEnvironment env, Func<DnnImageFeaturizerInput, EstimatorChain<ColumnCopyingTransformer>> modelFactory, string outputColumnName, string sourceColumnName = null)


IHostEnvironment env, Func<DnnImageFeaturizerInput, EstimatorChain> modelFactory, [](start = 43, length = 107)

outputColumnName should come first. #Resolved

sfilipi · 2019-01-25T07:09:15Z

        /// <param name="gpuDeviceId">Optional GPU device ID to run execution on. Null for CPU.</param>
        /// <param name="fallbackToCpu">If GPU error, raise exception or fallback to CPU.</param>
-        public OnnxTransformer(IHostEnvironment env, string modelFile, string inputColumn, string outputColumn, int? gpuDeviceId = null, bool fallbackToCpu = false)
+        public OnnxTransformer(IHostEnvironment env, string modelFile, string outputColumnName, string sourceColumnName = null, int? gpuDeviceId = null, bool fallbackToCpu = false)


string outputColumnName, [](start = 71, length = 24)

outputColumName should come first #Resolved

sfilipi · 2019-01-25T07:09:38Z

        /// <param name="gpuDeviceId">Optional GPU device ID to run execution on. Null for CPU.</param>
        /// <param name="fallbackToCpu">If GPU error, raise exception or fallback to CPU.</param>
-        public OnnxTransformer(IHostEnvironment env, string modelFile, string[] inputColumns, string[] outputColumns, int? gpuDeviceId = null, bool fallbackToCpu = false)
+        public OnnxTransformer(IHostEnvironment env, string modelFile, string[] outputColumnNames, string[] sourceColumnNames, int? gpuDeviceId = null, bool fallbackToCpu = false)


string[] outputColumnNames [](start = 70, length = 27)

outputColumnNames should come first #Resolved

sfilipi · 2019-01-25T07:10:27Z

        /// <param name="fallbackToCpu">If GPU error, raise exception or fallback to CPU.</param>
-        public OnnxScoringEstimator(IHostEnvironment env, string modelFile, string[] inputColumns, string[] outputColumns, int? gpuDeviceId = null, bool fallbackToCpu = false)
-           : this(env, new OnnxTransformer(env, modelFile, inputColumns, outputColumns, gpuDeviceId, fallbackToCpu))
+        public OnnxScoringEstimator(IHostEnvironment env, string modelFile, string[] names, string[] sources, int? gpuDeviceId = null, bool fallbackToCpu = false)


names [](start = 85, length = 5)

outputColumnName, and it also should be the first param. #Resolved

sfilipi · 2019-01-25T07:13:50Z

-        public ColumnConcatenatingEstimator (IHostEnvironment env, string outputColumn, params string[] inputColumns)
+        /// <param name="name">The name of the resulting column.</param>
+        /// <param name="sources">The columns to concatenate together.</param>
+        public ColumnConcatenatingEstimator (IHostEnvironment env, string name, params string[] sources)


sources [](start = 96, length = 7)

sourceColumnNames #Resolved

sfilipi · 2019-01-25T07:29:21Z

        }

-        public ColumnCopyingEstimator(IHostEnvironment env, params (string input, string output)[] columns)
+        public ColumnCopyingEstimator(IHostEnvironment env, params (string name, string sourceColumnName)[] columns)


name [](start = 75, length = 4)

outputColumnName #Resolved

sfilipi · 2019-01-25T07:44:27Z

-            public readonly string Input;
-            public readonly string Output;
+            public readonly string Name;
+            public readonly string Source;


Source [](start = 35, length = 6)

SourceColumnName #Resolved

sfilipi · 2019-01-25T07:59:26Z

                Contracts.Assert(toOutput.Length == 1);

-                var pairs = new List<(string[] inputs, string output)>();
+                var pairs = new List<(string name, string[] sources)>();


name [](start = 45, length = 4)

outputColumn #Resolved

sfilipi · 2019-01-25T08:00:55Z

            /// </summary>
            [Argument(ArgumentType.Multiple | ArgumentType.Required, HelpText = "The names of the model inputs", ShortName = "inputs", SortOrder = 1)]
-            public string[] InputColumns;
+            public string[] Sources;


Sources [](start = 28, length = 7)

Revert #Resolved

sfilipi · 2019-01-25T08:01:31Z

+        /// <param name="sources">The name of the input data columns. Must match model's input names.</param>
+        /// <param name="names">The output columns to generate. Names must match model specifications. Data types are inferred from model.</param>
+        public TensorFlowTransformer(IHostEnvironment env, TensorFlowModelInfo tfModelInfo, string[] names ,string[] sources)
+            : this(env, tfModelInfo.Session, names, sources, TensorFlowUtils.IsSavedModel(env, tfModelInfo.ModelPath) ? tfModelInfo.ModelPath : null, false)


outputColumnName #Resolved

sfilipi · 2019-01-25T08:02:49Z


-        public TensorFlowEstimator(IHostEnvironment env, string modelLocation, string[] inputs, string[] outputs)
-            : this(env, TensorFlowUtils.LoadTensorFlowModel(env, modelLocation), inputs, outputs)
+        public TensorFlowEstimator(IHostEnvironment env, string modelLocation, string[] names, string[] sources)


names [](start = 88, length = 5)

outputColumnNames #Resolved

sfilipi · 2019-01-25T08:03:03Z


-        public TensorFlowEstimator(IHostEnvironment env, TensorFlowModelInfo tensorFlowModel, string[] inputs, string[] outputs)
-            : this(env, CreateArguments(tensorFlowModel, inputs, outputs), tensorFlowModel)
+        public TensorFlowEstimator(IHostEnvironment env, TensorFlowModelInfo tensorFlowModel, string[] names, string[] sources)


sources [](start = 119, length = 7)

sourceColumnNames #Resolved

sfilipi · 2019-01-25T08:10:59Z

The deltas are suspicious, because i didn't add/remove any lines of code. I am only renaming parameter.

I wander if it is because of libmf being on a different hash. Will revert that.

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

TomFinley · 2019-01-28T18:14:53Z

            // values than the specified count. This transformation can be used to remove slots with too many missing values.
            var countSelectEst = ml.Transforms.FeatureSelection.SelectFeaturesBasedOnCount(
-                inputColumn: "Features", outputColumn: "FeaturesCountSelect", count: 695);
+                outputColumnName: "FeaturesCountSelect", sourceColumnName: "Features", count: 695);


Were we preferring the name "input" compared to "source" for the column names? Or is this exceptional for some reason? #Resolved

i think we agreed to sourceColumnName..

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

i think we agreed to sourceColumnName..

Are you sure @sfilipi ? Perhaps I am misinterpreting your summary of #2064, but it seems fairly clear to me. is this different for some other reasons?

the name of the column resulting from the transformation will be "outputColumnName" and the name of the column/columns to apply the transformation too would be "inputColumnName".
#Resolved

HA! this is how deep you had planted the "source" name in my mind!

changing it to be truthful to the discussion summary.

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

HA! this is how deep you had planted the "source" name in my mind!

😄 Now that I know I have this ability, I solemnly swear to use my special powers only for good.

(My own good, that is.)

sfilipi · 2019-01-28T21:58:32Z

@wschin, @zeahmed would you guys have time to take a look today?
thanks. #Resolved

TomFinley · 2019-01-28T23:11:18Z

Note that I added two tests from a recent commit whenever you sync again @sfilipi , CategoricalOneHotEncodingFromSideData and ValueToKeyFromSideData, that might get auto-merged, but you'll want to pay attention since they had things in the original order. #Resolved

zeahmed · 2019-01-29T02:29:02Z

[](start = 75, length = 2)

nit

sfilipi · 2019-01-29T06:01:10Z

-            string inputColumn,
-            string outputColumn = null,
+            string name,
+            string source = null,


fix #Resolved

sfilipi · 2019-01-29T06:01:35Z

-            string[] inputColumns,
-            string outputColumn,
+            string name,
+            string[] sources,


fix #Resolved

sfilipi · 2019-01-29T06:01:52Z

        /// <value>0</value> does not retain any input values. <value>-1</value> retains all input values mapping to each hash.</param>
        public NgramHashingEstimator(IHostEnvironment env,
-            (string[] inputs, string output)[] columns,
+            (string name, string[] sources)[] columns,


(string name, string[] sourc [](start = 11, length = 29)

fix #Resolved

sfilipi · 2019-01-29T06:03:29Z

                return input;

-            input = new ColumnCopyingTransformer(env, hiddenNames.Select(x => (Input: x.Key, Output: x.Value)).ToArray()).Transform(input);
+            input = new ColumnCopyingTransformer(env, hiddenNames.Select(x => (Name: x.Key, Source: x.Value)).ToArray()).Transform(input);


Source [](start = 92, length = 6)

fix #Resolved

zeahmed · 2019-01-29T19:13:26Z

@@ -178,13 +178,13 @@ private sealed class Mapper : OneToOneMapperBase
            private sealed class ColInfo


outputColumnName [](start = 38, length = 16)

name?

…classes, to be "sourceColumnName", as suggested. Changing the "name" parameter to "outputColumnName" in the: - estimator extension APIs - estimator ctors - column pairs expressed through tuples, because in context it reads better than name. Note: in the columnInfo classes i left it to "name" because "outputColumnName" makes no sense. 2 - Nit on standartizing the XML comments. 3 - Arranging the order of the parameters to be: outputColumnName, required parameters, nullable sourceColumnName.

reverting update to libmf fix typo in missingValueIndicatorTransformer

a few more name, source pairs changed

zeahmed · 2019-01-29T19:33:04Z

-        /// <param name="inputColumns">The input columns</param>
-        /// <param name="outputColumn">The output column</param>
+        /// <param name="outputColumnName">Name of the column resulting from the transformation of <paramref name="inputColumnNames"/>.</param>
+        /// <param name="inputColumnNames">Name of the column to transform. If set to <see langword="null"/>, the value of the <paramref name="outputColumnName"/> will be used as source.</param>


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

Columns?? #Resolved

zeahmed · 2019-01-29T19:33:25Z

-        /// <param name="inputColumns">The columns containing text to compute bag of word vector.</param>
-        /// <param name="outputColumn">The column containing output tokens.</param>
+        /// <param name="outputColumnName">Name of the column resulting from the transformation of <paramref name="inputColumnNames"/>.</param>
+        /// <param name="inputColumnNames">Name of the column to transform.</param>


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

Columns?? #Resolved

zeahmed · 2019-01-29T19:33:41Z

-        /// <param name="inputColumns">The columns containing text to compute bag of word vector.</param>
-        /// <param name="outputColumn">The column containing output tokens.</param>
+        /// <param name="outputColumnName">Name of the column resulting from the transformation of <paramref name="inputColumnNames"/>.</param>
+        /// <param name="inputColumnNames">Name of the column to transform. If set to <see langword="null"/>, the value of the <paramref name="outputColumnName"/> will be used as source.</param>


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

Columns?? #Resolved

zeahmed · 2019-01-29T19:33:54Z

-        /// <param name="inputColumns">Name of input columns containing tokenized text.</param>
-        /// <param name="outputColumn">Name of output column, will contain the ngram vector.</param>
+        /// <param name="outputColumnName">Name of the column resulting from the transformation of <paramref name="inputColumnNames"/>.</param>
+        /// <param name="inputColumnNames">Name of the column to transform.</param>


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

Columns?? #Resolved

zeahmed

Overall it looks good. I have left a few comments.

sfilipi mentioned this pull request Jan 25, 2019

swapping the order or arguments on the constructors of the ConversionExtensionsCatalog. Internalizing the constructors. #2118

Closed

sfilipi commented Jan 25, 2019

View reviewed changes

sfilipi requested review from Ivanidzo4ka, TomFinley, wschin and zeahmed January 27, 2019 09:27

sfilipi changed the title ~~WIP - Input output swap~~ Input output swap Jan 27, 2019

sfilipi self-assigned this Jan 28, 2019

TomFinley reviewed Jan 28, 2019

View reviewed changes

zeahmed reviewed Jan 29, 2019

View reviewed changes

sfilipi commented Jan 29, 2019

View reviewed changes

zeahmed reviewed Jan 29, 2019

View reviewed changes

sfilipi added 12 commits January 29, 2019 11:15

All the code changes in, and most of the tests updated.

b7d5d38

all the tests pass

cfcfdbb

post rebase fixes

805aac8

Addressing code comments, and post rebase fixes.

7cc7bbc

updating the baseline for wordbag after bug fix.

4a49fc4

reverting update to libmf fix typo in missingValueIndicatorTransformer

one more a miss

654bfdf

post rebase fixes

c213f85

fixing typo on the wordbag transform

e8c5cd3

a few more name, source pairs changed

mass rename of ;sourceColumnName" to "inputColumnName"

37f896a

fixing the benchmark test

22836d8

adapting to change

8006282

sfilipi force-pushed the inputOutputSwap branch from d219291 to 8006282 Compare January 29, 2019 19:23

zeahmed reviewed Jan 29, 2019

View reviewed changes

zeahmed approved these changes Jan 29, 2019

View reviewed changes

addressing PR comments

cc6df5c

sfilipi merged commit e383091 into dotnet:master Jan 29, 2019

sfilipi deleted the inputOutputSwap branch January 29, 2019 21:27

sfilipi mentioned this pull request Jan 30, 2019

Estimator arguments should take output column name as first parameter, any inputs as subsequent parameters #2064

Closed

TomFinley mentioned this pull request Feb 1, 2019

KeyToValueMapping API is inconsistent with rest #2376

Closed

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

		@@ -178,13 +178,13 @@ private sealed class Mapper : OneToOneMapperBase
		private sealed class ColInfo

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

sfilipi Jan 25, 2019 • 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 Jan 25, 2019 • 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 Jan 25, 2019 • 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 Jan 25, 2019 • 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 Jan 25, 2019 • 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 Jan 25, 2019 • 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 Jan 25, 2019 • 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 Jan 25, 2019 • 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 Jan 25, 2019 • 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 Jan 25, 2019 • 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 Jan 25, 2019 • 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 Jan 25, 2019 • 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 Jan 25, 2019 • 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 Jan 25, 2019 • 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 Jan 25, 2019 • 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 Jan 25, 2019 • 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 Jan 25, 2019 • 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 Jan 25, 2019 • 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 commented Jan 25, 2019 •

edited

Loading

codecov Bot commented Jan 25, 2019 •

edited

Loading

sfilipi Jan 25, 2019 •

edited

Loading

sfilipi Jan 25, 2019 •

edited

Loading

sfilipi Jan 25, 2019 •

edited

Loading

sfilipi Jan 25, 2019 •

edited

Loading

sfilipi Jan 25, 2019 •

edited

Loading

sfilipi Jan 25, 2019 •

edited

Loading

sfilipi Jan 25, 2019 •

edited

Loading

sfilipi Jan 25, 2019 •

edited

Loading

sfilipi Jan 25, 2019 •

edited

Loading

sfilipi Jan 25, 2019 •

edited

Loading

sfilipi Jan 25, 2019 •

edited

Loading

sfilipi Jan 25, 2019 •

edited

Loading

sfilipi Jan 25, 2019 •

edited

Loading

sfilipi Jan 25, 2019 •

edited

Loading

sfilipi Jan 25, 2019 •

edited

Loading

sfilipi Jan 25, 2019 •

edited

Loading

sfilipi Jan 25, 2019 •

edited

Loading

sfilipi Jan 25, 2019 •

edited

Loading

TomFinley Jan 28, 2019 •

edited by sfilipi

Loading

sfilipi Jan 28, 2019 •

edited

Loading

TomFinley Jan 29, 2019 •

edited by sfilipi

Loading

sfilipi commented Jan 28, 2019 •

edited

Loading

TomFinley commented Jan 28, 2019 •

edited by sfilipi

Loading

sfilipi Jan 29, 2019 •

edited

Loading

sfilipi Jan 29, 2019 •

edited

Loading

sfilipi Jan 29, 2019 •

edited

Loading

sfilipi Jan 29, 2019 •

edited

Loading

zeahmed Jan 29, 2019 •

edited by sfilipi

Loading

zeahmed Jan 29, 2019 •

edited by sfilipi

Loading

zeahmed Jan 29, 2019 •

edited by sfilipi

Loading

zeahmed Jan 29, 2019 •

edited by sfilipi

Loading