Conversion of NAIndicatorTransform to estimator with related pigstensions #1217

artidoro · 2018-10-10T21:48:03Z

Ongoing work on converting the transformers to estimators (#754). This PR converts the NA Indicator transform to estimator and adds the relative pigsty extensions.

…to naindicator

…re tests pass

…to naindicator

sfilipi · 2018-10-11T05:06:40Z

src/Microsoft.ML.Transforms/NAIndicatorTransform.cs

 using System.Text;
+using Microsoft.ML.Core.Data;
 using Microsoft.ML.Runtime;
 using Microsoft.ML.Runtime.CommandLine;
 using Microsoft.ML.Runtime.Data;
 using Microsoft.ML.Runtime.Data.Conversion;
 using Microsoft.ML.Runtime.EntryPoints;


if you Ctrl+R+G they will get re-ordered differently. #Resolved

sfilipi · 2018-10-11T05:07:45Z

src/Microsoft.ML.Transforms/NAIndicatorTransform.cs


 namespace Microsoft.ML.Runtime.Data
 {
    /// <include file='doc.xml' path='doc/members/member[@name="NAIndicator"]'/>
-    public sealed class NAIndicatorTransform : OneToOneTransformBase
+    public sealed class NAIndicatorTransform : OneToOneTransformerBase
    {
        public sealed class Column : OneToOneColumn


public [](start = 8, length = 6)

as we go through this, if you have time to add a one line comment on the public members, a they will appear in the documentation. #Resolved

Ok will do that!

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

sfilipi · 2018-10-11T05:09:16Z

src/Microsoft.ML.Transforms/NAIndicatorTransform.cs

        }

-        private const string RegistrationName = "NaIndicator";
+        private const string RegistrationName = nameof(NAIndicatorTransform);


nameof(NAIndicatorTransform); [](start = 48, length = 29)

The usage of nameof here is great! Shall we get rid of the LoadName and just substitute with this? #Resolved

Sounds good!

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

I disagree because of the above comment

In reply to: 224647220 [](ancestors = 224647220,224316082)

sfilipi · 2018-10-11T05:13:39Z

src/Microsoft.ML.Transforms/NAIndicatorTransform.cs

-        public NAIndicatorTransform(IHostEnvironment env, Arguments args, IDataView input)
-            : base(env, RegistrationName, Contracts.CheckRef(args, nameof(args)).Column,
-                input, TestType)
+        public static IDataTransform Create(IHostEnvironment env, Arguments args, IDataView input)


public [](start = 8, length = 6)

can it be internal? #Closed

even private!

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

Ivanidzo4ka · 2018-10-11T18:19:31Z

src/Microsoft.ML.Transforms/NAIndicatorTransform.cs

+    NAIndicatorTransform.FriendlyName, NAIndicatorTransform.LoadName)]
+
+[assembly: LoadableClass(typeof(IRowMapper), typeof(NAIndicatorTransform), null, typeof(SignatureLoadRowMapper),
+   NAIndicatorTransform.FriendlyName, NAIndicatorTransform.LoadName)]

 namespace Microsoft.ML.Runtime.Data


Runtime.Data [](start = 23, length = 12)

Transforms instead of Runtime.Data #Closed

Ivanidzo4ka · 2018-10-11T18:22:54Z

src/Microsoft.ML.Transforms/NAIndicatorTransform.cs

+        /// <summary>
+        /// Factory method for SignatureDataTransform.
+        /// </summary>
+        public static NAIndicatorTransform Create(IHostEnvironment env, ModelLoadContext ctx)


make all of them (factory methods) private if you can.
also I still prefer to have // comment instead of summary. #Closed

Sure, I thought they were going to be public that's why I added the summary, but I'll change to comment then.

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

Why is this not a constructor?

In reply to: 224647551 [](ancestors = 224647551,224554120)

Ivanidzo4ka · 2018-10-11T18:24:54Z

src/Microsoft.ML.Transforms/NAIndicatorTransform.cs

-                {
-                    // Output is normalized.
-                    bldr.AddPrimitive(MetadataUtils.Kinds.IsNormalized, BoolType.Instance, true);
-                }


I don't see this metadata been properly propagated in estimator/ mapper #Closed

Indeed. You need to have IsNormalized metadata

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

Zruty0 · 2018-10-12T17:32:14Z

test/Microsoft.ML.Tests/Transformers/NAIndicatorTests.cs

+        }
+
+        [Fact]
+        public void NAIndicatorStatic()


NAIndicatorStatic [](start = 20, length = 17)

Please move next to other static tests #Closed

We decided to group static tests together and other transformer tests together

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

Zruty0 · 2018-10-12T17:34:04Z

            // REVIEW: temporary name

remove #Closed

Refers to: src/Microsoft.ML.Transforms/NAIndicatorTransform.cs:67 in 76a893a. [](commit_id = 76a893a, deletion_comment = False)

Zruty0 · 2018-10-12T17:34:46Z

src/Microsoft.ML.Transforms/NAIndicatorTransform.cs

@@ -68,116 +78,196 @@ private static VersionInfo GetVersionInfo()
        internal const string FriendlyName = "NA Indicator Transform";
        internal const string ShortName = "NAInd";

-        private static string TestType(ColumnType type)
+        internal static string TestType(ColumnType type)
        {
            // Item type must have an NA value. We'll get the predicate again later when we're ready to use it.
            Delegate del;
            if (Conversions.Instance.TryGetIsNAPredicate(type.ItemType, out del))
                return null;
            return string.Format("Type '{0}' is not supported by {1} since it doesn't have an NA value",


Format [](start = 26, length = 6)

use $ syntax #Closed

Zruty0 · 2018-10-12T17:35:27Z

src/Microsoft.ML.Transforms/NAIndicatorTransform.cs

+                type, LoadName);
+        }
+
+        public class ColumnInfo


c class [](start = 13, length = 7)

sealed. Also, why do you need it? The base class is doing fine with just a (string input, string output) value tuple #Closed

Zruty0 · 2018-10-12T17:37:39Z

src/Microsoft.ML.Transforms/NAIndicatorTransform.cs

+                }
+            }
+            return (inputTypes, outputTypes);
+        }

        /// <summary>


summary [](start = 13, length = 7)

a) this summary should change, because it is not for the public facing API.
Also the param comments are bad.

b) this constructor should not be public at all. The only way to train a trainable transformer should be via estimator's Fit call, not directly via some constructor #Closed

Actually, I take it even further. This transformer should not be trainable at all. It should not memorize the column types, only names.
All of the column type stuff should belong to Mapper.

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

Zruty0 · 2018-10-12T17:42:07Z

src/Microsoft.ML.Transforms/NAIndicatorTransform.cs

-        private NAIndicatorTransform(IHost host, ModelLoadContext ctx, IDataView input)
-            : base(host, ctx, input, TestType)
+        /// <summary>
+        /// Factory method for SignatureDataTransform.


Factory method for SignatureDataTransform. [](start = 11, length = 43)

This is not true. This is a factory method for SignatureLoadModel #Closed

Zruty0 · 2018-10-12T17:42:49Z

src/Microsoft.ML.Transforms/NAIndicatorTransform.cs

+        /// <summary>
+        /// Factory method for SignatureDataTransform.
+        /// </summary>
+        public static IDataTransform Create(IHostEnvironment env, IDataView input, params ColumnInfo[] columns)


Create [](start = 37, length = 6)

It is not a factory method for anything, remove it altogether #Closed

Ivanidzo4ka · 2018-10-16T18:04:25Z

src/Microsoft.ML.Transforms/NAIndicatorTransform.cs

+                var metadata = new List<SchemaShape.Column>();
+                if (col.Metadata.TryFindColumn(MetadataUtils.Kinds.SlotNames, out var slotMeta))
+                    metadata.Add(slotMeta);
+                if (col.Metadata.TryFindColumn(MetadataUtils.Kinds.IsNormalized, out var normalized))


if (col.Metadata.TryFindColumn(MetadataUtils.Kinds.IsNormalized, out var normalized)) [](start = 15, length = 86)

I think previously it got added unconditionally, and here you check source column #Closed

Ivanidzo4ka · 2018-10-16T18:06:03Z

src/Microsoft.ML.Transforms/NAIndicatorTransform.cs

+                    InputSchema.TryGetColumnIndex(_infos[iinfo].Input, out int colIndex);
+                    Host.Assert(colIndex >= 0);
+                    var colMetaInfo = new ColumnMetadataInfo(_infos[iinfo].Output);
+                    var meta = RowColumnUtils.GetMetadataAsRow(InputSchema, colIndex, x => x == MetadataUtils.Kinds.SlotNames || x == MetadataUtils.Kinds.IsNormalized);


|| x == MetadataUtils.Kinds.IsNormalized [](start = 126, length = 40)

it should be unconditional. #Closed

Ivanidzo4ka · 2018-10-16T18:06:57Z

test/Microsoft.ML.Tests/Transformers/NAIndicatorTests.cs

+        }
+
+        [Fact]
+        public void TestOldSavingAndLoading()


Can you add small metadata test? #Closed

…to naindicator

Zruty0 · 2018-10-17T20:35:51Z

src/Microsoft.ML.Transforms/NAIndicatorTransform.cs

-            : this(env, new Arguments() { Column = new[] { new Column() { Source = source ?? name, Name = name } } }, input)
+        /// <param name="env">The environment to use.</param>
+        /// <param name="columns">The names of the input columns of the transformation and the corresponding names for the output columns.</param>
+        internal NAIndicatorTransform(IHostEnvironment env, params (string input, string output)[] columns)


internal [](start = 8, length = 8)

now it's safe to make this ctor public :) #Closed

Zruty0 · 2018-10-17T20:36:55Z

src/Microsoft.ML.Transforms/NAIndicatorTransform.cs


-            // *** Binary format ***
-            // <base>
+        private static (string input, string output)[] GetColumnPairs(Column[] columns)


GetColumnPairs [](start = 55, length = 14)

this entire method can be replaced with
columns.Select(c=>(c.Source ?? c.Name, c.Name)).ToArray() #Closed

Zruty0 · 2018-10-17T20:43:10Z

src/Microsoft.ML.Transforms/NAIndicatorTransform.cs

+                    Output = output;
+                    InputType = inType;
+                    OutputType = outType;
+                    InputIsNA = GetIsNADelegate(InputType); ;


; [](start = 59, length = 2)

remove #Closed

…to naindicator

Zruty0 · 2018-10-18T01:29:12Z

src/Microsoft.ML.Transforms/NAIndicatorTransform.cs


-        // The output column types, parallel to Infos.
-        private readonly ColumnType[] _types;
+        internal (string input, string output)[] GetColumnPairs() => ColumnPairs;


internal [](start = 8, length = 8)

The pattern is :
public IReadOnlyList<(string input, string output)> Columns => ColumnPairs.AsReadOnly() #Resolved

Zruty0 · 2018-10-18T01:30:19Z

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

@@ -84,7 +84,7 @@ public void KMeansEstimator()
        public void TestEstimatorHogwildSGD()
        {
            (IEstimator<ITransformer> pipe, IDataView dataView) = GetBinaryClassificationPipeline();
-            pipe.Append(new StochasticGradientDescentClassificationTrainer(Env, "Features", "Label"));
+            pipe = pipe.Append(new StochasticGradientDescentClassificationTrainer(Env, "Features", "Label"));


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

nice catch :) #Resolved

Zruty0

Ivanidzo4ka

Ivanidzo4ka

Ivanidzo4ka

Ivanidzo4ka

Ivanidzo4ka · 2018-10-19T22:56:45Z

test/Microsoft.ML.Tests/Transformers/NAIndicatorTests.cs

+
+            var dataView = ComponentCreation.CreateDataView(Env, data);
+            var args = new NAIndicatorTransform.Arguments();
+            args.Column = new NAIndicatorTransform.Column[] { new NAIndicatorTransform.Column() { Name = "NAA", Source = "A" } };


why you keep using arguments class? #Resolved

Good point, I will change this!

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

…ning into naindicator

…to naindicator

artidoro added 8 commits October 5, 2018 10:32

started conversion work

e0c85cd

started conversion work

9e23955

Merge branch 'master' of https://github.com/dotnet/machinelearning in…

11b7dd9

…to naindicator

wrote a first version of the conversion, need to debug it and make su…

e0e7d3b

…re tests pass

Merge branch 'master' of https://github.com/dotnet/machinelearning in…

593f7d5

…to naindicator

finished debugging tests

9c1e758

Merge branch 'master' of https://github.com/dotnet/machinelearning in…

5e20acd

…to naindicator

cleanup

4062bed

artidoro requested review from dotnet-bot, Ivanidzo4ka, singlis and sfilipi and removed request for dotnet-bot October 10, 2018 21:48

artidoro added the API Issues pertaining the friendly API label Oct 10, 2018

Zruty0 mentioned this pull request Oct 10, 2018

New API for ML.NET #754

Closed

fixing an issue

76a893a

sfilipi reviewed Oct 11, 2018

View reviewed changes

Ivanidzo4ka reviewed Oct 11, 2018

View reviewed changes

Zruty0 reviewed Oct 12, 2018

View reviewed changes

Ivanidzo4ka reviewed Oct 16, 2018

View reviewed changes

artidoro added 3 commits October 16, 2018 14:16

fixed entrypointcatalog test

122d48c

Merge branch 'master' of https://github.com/dotnet/machinelearning in…

ef8c999

…to naindicator

propagated metadata

28e9570

Zruty0 reviewed Oct 17, 2018

View reviewed changes

artidoro added 2 commits October 17, 2018 18:24

fixed review comments

f720a75

Merge branch 'master' of https://github.com/dotnet/machinelearning in…

90784fb

…to naindicator

Zruty0 reviewed Oct 18, 2018

View reviewed changes

Zruty0 approved these changes Oct 18, 2018

View reviewed changes

fixed review comments

5d4b6bc

artidoro closed this Oct 18, 2018

artidoro reopened this Oct 18, 2018

Merge branch 'master' into naindicator

dd49b4b

Ivanidzo4ka approved these changes Oct 19, 2018

View reviewed changes

Ivanidzo4ka reviewed Oct 19, 2018

View reviewed changes

artidoro added 3 commits October 19, 2018 16:26

removed unused object

0f4507f

Merge branch 'naindicator' of https://github.com/artidoro/machinelear…

25c9b9e

…ning into naindicator

Merge branch 'master' of https://github.com/dotnet/machinelearning in…

4a6a5d3

…to naindicator

artidoro merged commit 06179b8 into dotnet:master Oct 20, 2018

yaeldekel mentioned this pull request Oct 20, 2018

Fix merge break #1327

Merged

artidoro deleted the naindicator branch January 5, 2019 00:01

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

Conversion of NAIndicatorTransform to estimator with related pigstensions #1217

Conversion of NAIndicatorTransform to estimator with related pigstensions #1217

Conversation

artidoro commented Oct 10, 2018

sfilipi Oct 11, 2018 • edited by artidoro Loading

Choose a reason for hiding this comment

sfilipi Oct 11, 2018 • edited by artidoro Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Oct 11, 2018 • edited by artidoro Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Oct 11, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Oct 11, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ivanidzo4ka Oct 11, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 Oct 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 commented Oct 12, 2018 • edited Loading

Zruty0 Oct 12, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Oct 12, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Oct 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 Oct 12, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Oct 12, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Oct 16, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Oct 16, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Oct 16, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Oct 18, 2018 • edited by artidoro Loading

Choose a reason for hiding this comment

Zruty0 Oct 18, 2018 • edited by artidoro Loading

Choose a reason for hiding this comment

Zruty0 left a comment

Choose a reason for hiding this comment

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

Ivanidzo4ka Oct 19, 2018 • edited by artidoro Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfilipi Oct 11, 2018 •

edited by artidoro

Loading

sfilipi Oct 11, 2018 •

edited by artidoro

Loading

sfilipi Oct 11, 2018 •

edited by artidoro

Loading

sfilipi Oct 11, 2018 •

edited by Zruty0

Loading

Ivanidzo4ka Oct 11, 2018 •

edited

Loading

Ivanidzo4ka Oct 11, 2018 •

edited by Zruty0

Loading

Ivanidzo4ka Oct 11, 2018 •

edited by Zruty0

Loading

Zruty0 Oct 12, 2018 •

edited

Loading

Zruty0 commented Oct 12, 2018 •

edited

Loading

Zruty0 Oct 12, 2018 •

edited

Loading

Zruty0 Oct 12, 2018 •

edited

Loading

Zruty0 Oct 12, 2018 •

edited

Loading

Zruty0 Oct 12, 2018 •

edited

Loading

Zruty0 Oct 12, 2018 •

edited

Loading

Ivanidzo4ka Oct 16, 2018 •

edited

Loading

Ivanidzo4ka Oct 16, 2018 •

edited

Loading

Ivanidzo4ka Oct 16, 2018 •

edited

Loading

Zruty0 Oct 17, 2018 •

edited

Loading

Zruty0 Oct 17, 2018 •

edited

Loading

Zruty0 Oct 17, 2018 •

edited

Loading

Zruty0 Oct 18, 2018 •

edited by artidoro

Loading

Zruty0 Oct 18, 2018 •

edited by artidoro

Loading

Ivanidzo4ka Oct 19, 2018 •

edited by artidoro

Loading