Refactoring of Options for ImagePixelExtractingEstimator #3033

artidoro · 2019-03-20T02:00:24Z

This PR is an example solution for #2884. Once I receive feedback on this, I will continue with the rest of the transforms.

The purpose of this PR is twofold:

does the ground work to more easily enable the refactoring of Options in other transforms (commit 1)
- most of the work is in the file ColumnBindingsBase.cs.
- made OneToOneColumn public, removed empty class
- renamed Name and Source to OutputColumnName and InputColumnName
- added implicit operators to OneToOneColumn to simplify the multicolumn mapping scenario
refactors the Options class for ImagePixelExtractingEstimator (commit 2)
- moved immutable ColumnOptions to transformer and renamed ColumnInfo
- moved Options and Column to estimator
- refactored extension and constructors

The third commit fixes tests and entrypoint catalog.

Note that with the combination of the implicit operators on OneToOneColumn and the constructor taking OneToOneColumn in the Options class, it is easier to define the multicolumn mapping scenario where the columns don't require column specific settings. For an example of the behavior see the test TestImagePixelExtractOptions in ImagesTests.cs:

// options1 and 2 should be exactly the same.
var options1 = new ImagePixelExtractingEstimator.Options
{
    ColumnOptions = new[]
    {
        new ImagePixelExtractingEstimator.ColumnOptions { OutputColumnName = "outputColumn1", InputColumnName = "inputColumn1" },
        new ImagePixelExtractingEstimator.ColumnOptions { OutputColumnName = "outputColumn2", InputColumnName = "inputColumn2" },
        new ImagePixelExtractingEstimator.ColumnOptions { OutputColumnName = "outputColumn3", InputColumnName = "inputColumn3" }
    }
};
var options2 = new ImagePixelExtractingEstimator.Options(("outputColumn1", "inputColumn1"), ("outputColumn2", "inputColumn2"), ("outputColumn3", "inputColumn3"));
// options3 has the same OutputColumnName as the previous two.
var options3 = new ImagePixelExtractingEstimator.Options("outputColumn1", "outputColumn2", "outputColumn3");

src/Microsoft.ML.Data/Transforms/ColumnBindingsBase.cs

src/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs

codecov · 2019-03-20T03:11:14Z

Codecov Report

Merging #3033 into master will increase coverage by <.01%.
The diff coverage is 75.55%.

@@            Coverage Diff             @@
##           master    #3033      +/-   ##
==========================================
+ Coverage   72.41%   72.42%   +<.01%     
==========================================
  Files         803      803              
  Lines      143851   143863      +12     
  Branches    16173    16175       +2     
==========================================
+ Hits       104171   104188      +17     
+ Misses      35258    35256       -2     
+ Partials     4422     4419       -3

Flag	Coverage Δ
#Debug	`72.42% <75.55%> (ø)`	⬆️
#production	`68.09% <62.58%> (ø)`	⬆️
#test	`88.62% <100%> (ø)`	⬆️

Impacted Files	Coverage Δ
...ft.ML.ImageAnalytics/EntryPoints/ImageAnalytics.cs	`0% <ø> (ø)`	⬆️
...osoft.ML.Transforms/Text/TokenizingByCharacters.cs	`95.32% <0%> (ø)`	⬆️
...rc/Microsoft.ML.Transforms/HashJoiningTransform.cs	`85.75% <0%> (ø)`	⬆️
src/Microsoft.ML.Transforms/Text/WordTokenizing.cs	`79.15% <0%> (ø)`	⬆️
src/Microsoft.ML.ImageAnalytics/ImageLoader.cs	`84.55% <0%> (ø)`	⬆️
src/Microsoft.ML.Transforms/KeyToVectorMapping.cs	`91.24% <0%> (ø)`	⬆️
...rosoft.ML.Data/Transforms/LabelConvertTransform.cs	`15.51% <0%> (ø)`	⬆️
src/Microsoft.ML.ImageAnalytics/ImageResizer.cs	`84.64% <0%> (ø)`	⬆️
src/Microsoft.ML.Data/Transforms/NormalizeUtils.cs	`43.63% <0%> (ø)`	⬆️
...oft.ML.Transforms/Text/TextFeaturizingEstimator.cs	`83.2% <0%> (ø)`	⬆️
... and 48 more

sfilipi · 2019-03-20T05:21:29Z

src/Microsoft.ML.Data/Transforms/TransformBase.cs

-                    if (!inputSchema.TryGetColumnIndex(item.Source, out colSrc))
-                        throw host.ExceptUserArg(nameof(OneToOneColumn.Source), "Source column '{0}' not found", item.Source);
+                    if (!inputSchema.TryGetColumnIndex(item.InputColumnName, out colSrc))
+                        throw host.ExceptUserArg(nameof(OneToOneColumn.InputColumnName), "Source column '{0}' not found", item.InputColumnName);


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

Input column

sfilipi

TomFinley · 2019-03-20T17:19:02Z

src/Microsoft.ML.Data/Transforms/NormalizeColumn.cs

@@ -290,8 +290,8 @@ internal static IDataTransform Create(IHostEnvironment env, MinMaxArguments args

            var columns = args.Columns
                .Select(col => new NormalizingEstimator.MinMaxColumnOptions(
-                    col.Name,
-                    col.Source ?? col.Name,
+                    col.OutputColumnName,


OutputColumnName [](start = 24, length = 16)

I don't think that this is necessarily important, but in prior discussions we've discussed that in the case of a column options specifically, given that it's clear this is talking about columns we have discussed that it may be wise to name it InputName as opposed to InputColumnName, since it's obvious we're talking about columns here.

wschin · 2019-03-20T17:21:32Z

src/Microsoft.ML.Data/Transforms/ColumnBindingsBase.cs

+    /// <summary>
+    /// Specifies input and output column names for a transformation
+    /// </summary>
+    public class OneToOneColumn


OneToOneColumn [](start = 17, length = 14)

Is OneToOneColumn a good name? Usually one-to-one describes a mapping or a function, which doesn't sound a Column. Maybe we could try OneToOneBind?

src/Microsoft.ML.Data/Transforms/ColumnBindingsBase.cs

wschin · 2019-03-20T17:23:39Z

src/Microsoft.ML.Data/Transforms/ColumnBindingsBase.cs

+        /// <summary>
+        /// Instantiates a <see cref="ColumnOptions"/> from a tuple of input and output column names.
+        /// </summary>
+        public static implicit operator OneToOneColumn((string outputColumnName, string inputColumnName) value)


(string outputColumnName, string inputColumnName) value [](start = 55, length = 55)

I have an impression that tuples are not very welcomed.

TomFinley · 2019-03-20T20:54:51Z

src/Microsoft.ML.Data/Transforms/ColumnBindingsBase.cs

+        public string OutputColumnName;
+
+        /// <summary>Name of the column to transform. If set to <see langword="null"/>, the value of the <see cref= "OutputColumnName"/> will be used as source.</summary>
+        [Argument(ArgumentType.AtMostOnce, HelpText = "Name of the source column", ShortName = "source, src")]


ShortName [](start = 83, length = 9)

Note that you could have used the Name field here. That might have been preferable if we care about not changing the behavior of command line and entry-points, at least not yet. (We can always adjust later if we really want to.) #Resolved

Ok will use Name = .... to prevent further changes

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

TomFinley · 2019-03-20T20:55:14Z

src/Microsoft.ML.Data/Transforms/ColumnBindingsBase.cs

+
+        /// <summary>Name of the column to transform. If set to <see langword="null"/>, the value of the <see cref= "OutputColumnName"/> will be used as source.</summary>
+        [Argument(ArgumentType.AtMostOnce, HelpText = "Name of the source column", ShortName = "source, src")]
+        public string InputColumnName = null;


null [](start = 40, length = 4)

This null declaration serves no purpose. The default value of the field is null if unassigned, as we see above with OutputColumnName. #Resolved

TomFinley · 2019-03-20T20:58:26Z

src/Microsoft.ML.Data/Transforms/ColumnBindingsBase.cs

@@ -14,16 +14,40 @@

 namespace Microsoft.ML.Data
 {
-    internal abstract class SourceNameColumnBase
+    public class OneToOneColumn


OneToOneColumn [](start = 17, length = 14)

Do you mean this class to be used and directly instantiable? I would really not like that. I would prefer that our API be over sealed classes -- otherwise we'll have weird things like it being possible to assign OneToOneColumn taking APIs to mismatching descending classes.

As a general policy, we prefer all classes to be either abstract or sealed. We have generally followed this advice. #Resolved

I am making it abstract, thanks for pointing it out.

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

TomFinley · 2019-03-20T21:04:45Z

src/Microsoft.ML.Data/Transforms/ColumnBindingsBase.cs

+        /// <summary>
+        /// Instantiates a <see cref="ColumnOptions"/> from the column name.
+        /// </summary>
+        public static implicit operator OneToOneColumn(string outputColumnName)


implicit [](start = 22, length = 8)

I get a little nervous when I see implicit operators from common types, especially since in the same APIs that we're going to be using these sorts of types we're going to have string parameters being used. We could sort of get away with it with the value-tuples above -- we don't use value-tuples anywhere else -- but this makes me very nervous. #Resolved

Ok will remove the implicit operators, and simply have a constructor in the options class that will take tuples of strings.

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

TomFinley · 2019-03-20T21:05:59Z

src/Microsoft.ML.Data/Transforms/ColumnBindingsBase.cs

+            return new OneToOneColumn(outputColumnName);
+        }
+
+        public OneToOneColumn(string outputColumnName, string inputColumnName = null)


OneToOneColumn [](start = 15, length = 14)

This constructor is public. Note that we have a non-sealed directly instantiable class... #Resolved

Ok will remove the constructor

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

TomFinley

Let's start with the obvious, we need the class structure to change. But, this PR makes me very nervous. We're undertaking an expansion of our public API in a novel direction when we have, at best, four working days to detect any issues, and no clear compelling scenario to directly inform whether we're making the right choices or not. That just seems rather unwise to me, even if this PR had no problems.

glebuk · 2019-03-20T22:53:01Z

The concern of not taking this PR is that users will have to specify a transform per column. This can result in N*M slowdown where N is number of columns and M is the number of trainable transforms per column. The most common example is Categorical, We would strongly prefer to fit all the categories for all columns in a single pass.
If we ship v1.0 with no such mechanism to specify multiple columns per transform then we would leave >10x perf on the table and encourage a large set of users to use a sub-optimal patterns.
A secondary benefit would be to expose the common surface for the C# API, Nimbus and other entrypoint-based surface areas of ML.NET

If there is a simple way to expose just the multi-column support, but not the rest of the arg object, that would be a fine solution at this time.

artidoro · 2019-03-20T23:23:42Z

Closing in favor of a simpler approach prior to v1.

sfilipi · 2019-03-21T04:59:53Z

Been thinking that the multicolumn case might be more common than we think, for a few transforms, after seeing this PR: https://github.com/dotnet/machinelearning-samples/pull/321/files#diff-5f3b1d72514e8cdce1dcac64aaf8a097

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

artidoro added 2 commits March 19, 2019 18:46

renamed and refactored ColumnBindingsBase

3357699

ImagePixelExtracting Options refactoring

5d21541

artidoro added the API Issues pertaining the friendly API label Mar 20, 2019

artidoro added this to the 0319 milestone Mar 20, 2019

artidoro self-assigned this Mar 20, 2019

artidoro requested review from codemzs, wschin, TomFinley, abgoswam, singlis, sfilipi and rogancarr March 20, 2019 02:00

artidoro commented Mar 20, 2019

View reviewed changes

src/Microsoft.ML.Data/Transforms/ColumnBindingsBase.cs Outdated Show resolved Hide resolved

artidoro commented Mar 20, 2019

View reviewed changes

src/Microsoft.ML.Data/Transforms/ColumnBindingsBase.cs Outdated Show resolved Hide resolved

artidoro commented Mar 20, 2019

View reviewed changes

src/Microsoft.ML.ImageAnalytics/ExtensionsCatalog.cs Outdated Show resolved Hide resolved

artidoro added 2 commits March 19, 2019 19:35

tests and entrypoint catalogs

78d33bb

review comments

1d790bc

artidoro force-pushed the pixelextractoptions branch from 48c0f33 to 1d790bc Compare March 20, 2019 02:35

sfilipi reviewed Mar 20, 2019

View reviewed changes

sfilipi approved these changes Mar 20, 2019

View reviewed changes

TomFinley reviewed Mar 20, 2019

View reviewed changes

wschin reviewed Mar 20, 2019

View reviewed changes

src/Microsoft.ML.Data/Transforms/ColumnBindingsBase.cs Outdated Show resolved Hide resolved

wschin reviewed Mar 20, 2019

View reviewed changes

TomFinley reviewed Mar 20, 2019

View reviewed changes

TomFinley suggested changes Mar 20, 2019

View reviewed changes

review comments

04f2949

artidoro closed this Mar 20, 2019

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring of Options for ImagePixelExtractingEstimator #3033

Refactoring of Options for ImagePixelExtractingEstimator #3033

artidoro commented Mar 20, 2019

codecov bot commented Mar 20, 2019 •

edited

Loading

sfilipi Mar 20, 2019

sfilipi left a comment

TomFinley Mar 20, 2019

wschin Mar 20, 2019

wschin Mar 20, 2019

TomFinley Mar 20, 2019 •

edited by artidoro

Loading

artidoro Mar 20, 2019

TomFinley Mar 20, 2019 •

edited by artidoro

Loading

TomFinley Mar 20, 2019 •

edited by artidoro

Loading

artidoro Mar 20, 2019

TomFinley Mar 20, 2019 •

edited by artidoro

Loading

artidoro Mar 20, 2019

TomFinley Mar 20, 2019 •

edited by artidoro

Loading

artidoro Mar 20, 2019

TomFinley left a comment •

edited

Loading

glebuk commented Mar 20, 2019 •

edited

Loading

artidoro commented Mar 20, 2019

sfilipi commented Mar 21, 2019

Refactoring of Options for ImagePixelExtractingEstimator #3033

Refactoring of Options for ImagePixelExtractingEstimator #3033

Conversation

artidoro commented Mar 20, 2019

codecov bot commented Mar 20, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

sfilipi left a comment

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

TomFinley Mar 20, 2019 • edited by artidoro Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley Mar 20, 2019 • edited by artidoro Loading

Choose a reason for hiding this comment

TomFinley Mar 20, 2019 • edited by artidoro Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley Mar 20, 2019 • edited by artidoro Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley Mar 20, 2019 • edited by artidoro Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley left a comment • edited Loading

Choose a reason for hiding this comment

glebuk commented Mar 20, 2019 • edited Loading

artidoro commented Mar 20, 2019

sfilipi commented Mar 21, 2019

codecov bot commented Mar 20, 2019 •

edited

Loading

TomFinley Mar 20, 2019 •

edited by artidoro

Loading

TomFinley Mar 20, 2019 •

edited by artidoro

Loading

TomFinley Mar 20, 2019 •

edited by artidoro

Loading

TomFinley Mar 20, 2019 •

edited by artidoro

Loading

TomFinley Mar 20, 2019 •

edited by artidoro

Loading

TomFinley left a comment •

edited

Loading

glebuk commented Mar 20, 2019 •

edited

Loading