Implementing copy column estimator #706

Ivanidzo4ka · 2018-08-21T23:23:13Z

@Zruty0

Ivanidzo4ka · 2018-08-21T23:24:16Z

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

+            // *** Binary format ***
+            // int: number of added columns
+            // for each added column
+            //   int: id of output column name


int: id [](start = 16, length = 8)

string, not int #Closed

Zruty0 · 2018-08-21T23:26:31Z

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

@@ -45,6 +45,12 @@ public Column(string name, VectorKind vecKind, DataKind itemKind, bool isKey, st
                IsKey = isKey;
                MetadataKinds = metadataKinds;
            }
+
+            public Column CloneWithNewName(string newName)


CloneWithNewName [](start = 26, length = 16)

maybe this calls for mutable Column object?
If we don't do this, I guess you just have to write line 52 everywhere :) #Closed

Zruty0 · 2018-08-21T23:28:02Z

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

+                    throw Contracts.ExceptUserArg(nameof(columns), $"New column {column.Name} specified multiple times");
+                newNames.Add(column.Name);
+            }
+            _columns = columns;


columns.Select(x=>x.Name).Distinct().Count #Closed

I would prefer to tell user where he make mistake.

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

Zruty0 · 2018-08-21T23:29:04Z

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

+        private readonly IHost _host;
+
+        public CopyColumnsEstimator(IHostEnvironment env, string input, string output)
+        {


{ [](start = 8, length = 1)

Check env, then register host. then host.Check() everywhere #Closed

Zruty0 · 2018-08-21T23:29:27Z

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

+            _host = env.Register("CopyColumnsEstimator");
+        }
+
+        public CopyColumnsEstimator(IHostEnvironment env, (string Source, string Name)[] columns)


CopyColumnsEstimator [](start = 15, length = 20)

params? #Closed

what params?

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

make these a params (Source, Name)[] argument

In reply to: 212126225 [](ancestors = 212126225,211790834)

Zruty0 · 2018-08-21T23:29:40Z

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

+
+        public CopyColumnsTransformer Fit(IDataView input)
+        {
+            // invoke schema validation.


invoke [](start = 15, length = 6)

Invoke #Closed

Zruty0 · 2018-08-21T23:30:03Z

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

+
+        public SchemaShape GetOutputSchema(SchemaShape inputSchema)
+        {
+            Contracts.CheckValue(inputSchema, nameof(inputSchema));


Contracts [](start = 12, length = 9)

_host #Closed

Zruty0 · 2018-08-21T23:30:19Z

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

+        public SchemaShape GetOutputSchema(SchemaShape inputSchema)
+        {
+            Contracts.CheckValue(inputSchema, nameof(inputSchema));
+            Contracts.CheckValue(inputSchema.Columns, nameof(inputSchema.Columns));


Columns [](start = 45, length = 7)

No need. A valid schema will always have columns #Closed

Zruty0 · 2018-08-21T23:31:08Z

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

+                    throw Contracts.ExceptUserArg(nameof(inputSchema), $"{pair.source} not found in {nameof(inputSchema)}");
+                }
+            }
+            var result = new SchemaShape(originDic.Values.ToArray());


originDic [](start = 41, length = 9)

the other one, right? #Closed

Zruty0 · 2018-08-21T23:31:47Z

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

+        {
+            Contracts.CheckValue(inputSchema, nameof(inputSchema));
+            Contracts.CheckValue(inputSchema.Columns, nameof(inputSchema.Columns));
+            var originDic = inputSchema.Columns.ToDictionary(x => x.Name);


originDic [](start = 16, length = 9)

this is all supplanted by schema.FindColumn #Closed

Zruty0 · 2018-08-21T23:35:30Z

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

+
+            }
+
+            public Delegate[] CreateGetters(IRow input, Func<int, bool> activeOutput, out Action disposer)


input [](start = 49, length = 5)

need to assert that input.Schema matches _schema #Closed

Zruty0 · 2018-08-21T23:38:08Z

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

+
+            private Delegate MakeGetter<T>(IRow row, int src) => row.GetGetter<T>(src);
+
+            public Func<int, bool> GetDependencies(Func<int, bool> activeOutput) => (col) => (activeOutput(col) && _originalColumnSources.Contains(col));


GetDependencies [](start = 35, length = 15)

this is wrong: the argument to activeOutput is between 0 and GreateGetters().Length. The argument to the resulting Func is between 0 and _schema.ColumnCount #Closed

I suspect you use _colMap as old-to-new mapping, but in reality you populate the other way round.

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

I need both I guess

In reply to: 212480267 [](ancestors = 212480267,211792129)

Old to new is one-to-many

In reply to: 212482646 [](ancestors = 212482646,212480267,211792129)

Zruty0 · 2018-08-21T23:39:21Z

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

+            public RowMapperColumnInfo[] GetOutputColumns()
+            {
+                var result = new RowMapperColumnInfo[_columns.Length];
+                // Do I need return all columns or only output???


output [](start = 56, length = 6)

only output #Closed

Zruty0 · 2018-08-21T23:47:44Z

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

+                for (int i = 0; i < _columns.Length; i++)
+                {
+                    _schema.TryGetColumnIndex(_columns[i].source, out int colIndex);
+                    // how to get metadata?


let's add a REVIEW for now, that we want metadata to become IRow.
For now, pre-populate this output in ctor by pulling metadata from src schema #Closed

Zruty0 · 2018-08-21T23:49:58Z

[assembly: LoadableClass(CopyColumnsTransform.Summary, typeof(CopyColumnsTransform), typeof(CopyColumnsTransform.Arguments), typeof(SignatureDataTransform),

replace this with a Create method that manufactures a Transformer and wraps it's Transform into a RowToRowTransform #Closed

Refers to: src/Microsoft.ML.Data/Transforms/CopyColumnsTransform.cs:18 in 039653c. [](commit_id = 039653c, deletion_comment = False)

Zruty0 · 2018-08-21T23:52:32Z

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

@@ -18,6 +21,9 @@
 [assembly: LoadableClass(CopyColumnsTransform.Summary, typeof(CopyColumnsTransform), null, typeof(SignatureLoadDataTransform),


LoadableClass [](start = 11, length = 13)

remove altogether

Add loadable class for the mapper instead #Closed

with SignatureLoadRowMapper as a signature.

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

Zruty0 · 2018-08-21T23:59:57Z

test/Microsoft.ML.Tests/CopyColumnEstimatorTests.cs

+                {
+                    failed = true;
+                }
+                Assert.True(failed);


Assert [](start = 16, length = 6)

can you just make it Assert.True(false) inside the try block? #Closed

No, I want to validate what it actually failed. Your suggestion applicable if I want validate absence of failure.

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

I reject your rejection. Just put Assert.False() inside the try (not catch) block after the code that is supposed to throw

In reply to: 212474277 [](ancestors = 212474277,211795480)

Zruty0 · 2018-08-22T00:00:40Z

test/Microsoft.ML.Tests/CopyColumnEstimatorTests.cs

+        [Fact]
+        void TestBadTransformSchmea()
+        {
+            var data = new List<TestClass>() { new TestClass() { A = 1, B = 2, C = 3, }, new TestClass() { A = 4, B = 5, C = 6 } };


new [](start = 23, length = 3)

can new[] work here? #Closed

Zruty0 · 2018-08-22T00:13:52Z

test/Microsoft.ML.Tests/CopyColumnEstimatorTests.cs

+
+namespace Microsoft.ML.Tests
+{
+    public class CopyColumnEstimatorTests


CopyColumnEstimatorTests [](start = 17, length = 24)

The transform behavior is checked in TestDataPipeBase.TestCore. So we need to only check the estimator behavior: that we throw at schema errors for example.

I expect we need to create an estimator workout test as well, similar to TestCore. Not in this PR though.
#Closed

Zruty0 · 2018-08-23T23:33:12Z

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

+                    throw _host.ExceptParam(nameof(schema), $"{_columns[i].Source} not found in {nameof(schema)}");
+                }
+                _colNewToOldMapping.Add(i, colIndex);
+                _colOldToNewMapping.Add(i, colIndex);


_colOldToNewMapping.Add(i, colIndex); [](start = 16, length = 37)

I suppose you meant the reverse?

Also note that the same source can map to more than one target. I think it's better to have the new-to-old only (which is one to one), and build a bool array of dependencies inside GetDependencies #Closed

Zruty0 · 2018-08-23T23:34:17Z

using Microsoft.ML.Runtime;

header #Closed

Refers to: test/Microsoft.ML.Tests/CopyColumnEstimatorTests.cs:1 in 3d76715. [](commit_id = 3d76715, deletion_comment = False)

Zruty0

validate active output function in getter creation.

Zruty0

TomFinley · 2018-08-24T20:42:08Z

src/Microsoft.ML/CSharpApi.cs

-                var list = Column == null ? new List<Microsoft.ML.Transforms.CopyColumnsTransformColumn>() : new List<Microsoft.ML.Transforms.CopyColumnsTransformColumn>(Column);
-                list.Add(OneToOneColumn<Microsoft.ML.Transforms.CopyColumnsTransformColumn>.Create(inputColumn));
+                var list = Column == null ? new List<Microsoft.ML.Transforms.CopyColumnsTransformerColumn>() : new List<Microsoft.ML.Transforms.CopyColumnsTransformerColumn>(Column);
+                list.Add(OneToOneColumn<Microsoft.ML.Transforms.CopyColumnsTransformerColumn>.Create(inputColumn));


CopyColumnsTransformerColumn [](start = 64, length = 28)

This is a little unfortunate, maybe there is a way we can avoid this rename, since this is still our public facing API at least in the immediate term. #Resolved

I believe you've solved this based on a rename

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

TomFinley · 2018-08-24T20:45:05Z

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

+            {
+                if (newNames.Contains(column.Name))
+                    throw Contracts.ExceptUserArg(nameof(columns), $"New column {column.Name} specified multiple times");
+                newNames.Add(column.Name);


Note that HashSet<>.Add will itself return false if the element was already present in the set. Therefore this check can be written more efficiently as:

if (newNames.Add(column.Name) throw ...; ``` #Resolved

TomFinley · 2018-08-24T20:55:39Z

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

+        {
+        }
+
+        public CopyColumnsEstimator(IHostEnvironment env, params (string Source, string Name)[] columns)


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

From what I've seen in MSDN documentation on ValueTuple, while it's a little strange they seem to eschew normal .NET naming conventions on objects when used in this context and instead cleave to .NET naming conventions on parameters, since despite being a public member name it is being used kinda as a parameter name. So maybe source and name? #Resolved

Maybe RoboTom should be updated to be ValueTuple aware... not in this PR to be clear, just sometime.

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

TomFinley · 2018-08-24T20:56:52Z

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

+        private readonly IHost _host;
+
+        public CopyColumnsEstimator(IHostEnvironment env, string input, string output) :
+            this(env, new (string, string)[1] { (input, output) })


new (string, string)[1] { (input, output) } [](start = 22, length = 43)

It's a params, right? So could we do this(env, (input, output)) instead of this long thing? #Resolved

TomFinley · 2018-08-24T20:59:37Z

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

@@ -925,18 +925,10 @@ protected override IDataView GetOverallResultsCore(IDataView overall)

        private IDataView ChangeTopKAccColumnName(IDataView input)
        {
-            var cpyArgs = new CopyColumnsTransform.Arguments
+            input = new CopyColumnsTransformer(Host, new[]
            {


Since it's params, was it necessary to create this new array? Indeed since it is one item, could we not just have used the alternate non-params constructor? #Resolved

+ some pr comments

TomFinley

(dotnet#706) This builds on the Estimator conversion for the CopyColumnsTransform. This change is mainly refactoring as common code has moved to base level classes. This change is the following: - CopyColumnTransform now derives from OneToOneTransformerBase - CopyColumnEstimator now derives from TrivialEstimator - CopyColumnTransform::Mapper now derives from MapperBase - Removed code that was no longer needed due to these changes

) (#1141) * Updating the CopyColumnsEstimator and Transform to use common code (#706) This builds on the Estimator conversion for the CopyColumnsTransform. This change is mainly refactoring as common code has moved to base level classes. This change is the following: - CopyColumnTransform now derives from OneToOneTransformerBase - CopyColumnEstimator now derives from TrivialEstimator - CopyColumnTransform::Mapper now derives from MapperBase - Removed code that was no longer needed due to these changes * - Moved CopyColumnsTransform into Microsoft.ML.Transforms namespace, updated namespace usage and entrypoints due to this change. - Save now uses the SaveColumns from the base class - Other various changes based upon feedback.

Ivan Matantsev added 7 commits August 20, 2018 12:29

initial

4d39735

add tests, add more beef

9a0a03a

more tests

05012af

Merge branch 'master' into ivanidze/copy_column_estimator

ba52135

merge with master

d942001

one small change

a22914c

save and load

039653c

Ivanidzo4ka requested a review from Zruty0 August 21, 2018 23:23

Ivanidzo4ka commented Aug 21, 2018

View reviewed changes

Zruty0 reviewed Aug 21, 2018

View reviewed changes

Zruty0 reviewed Aug 22, 2018

View reviewed changes

Ivan Matantsev added 3 commits August 22, 2018 09:50

save and load old style.

77bf982

metadata pain!

03f923b

some code improvments

1587b13

fix build

3d76715

Zruty0 reviewed Aug 23, 2018

View reviewed changes

comments

2620f0c

Ivanidzo4ka requested a review from TomFinley August 24, 2018 00:22

Ivan Matantsev added 2 commits August 23, 2018 17:23

header!

fbe349b

sealed CopyColumnsRowMapper

e2a0711

Zruty0 approved these changes Aug 24, 2018

View reviewed changes

Ivan Matantsev added 5 commits August 24, 2018 08:35

Merge branch 'master' into ivanidze/copy_column_estimator

1fd6ae8

update EP

9341684

validate active output function in getter creation.

update CSharpApi

f2b6e72

fix build

5895e2a

stop being silly Ivan!

ca15330

Zruty0 approved these changes Aug 24, 2018

View reviewed changes

TomFinley reviewed Aug 24, 2018

View reviewed changes

Ivan Matantsev added 2 commits August 27, 2018 11:15

From transformer to transform

bd919ca

+ some pr comments

decapitalization of params

e85789c

TomFinley approved these changes Aug 27, 2018

View reviewed changes

Ivan Matantsev added 2 commits August 27, 2018 12:42

remains of csharpapi

30a88f5

make it buildable

e91b18f

Ivanidzo4ka merged commit 4ba15b8 into dotnet:master Aug 27, 2018

Zruty0 mentioned this pull request Aug 27, 2018

New API for ML.NET #754

Closed

sfilipi mentioned this pull request Oct 10, 2018

API reference - Samples for Transforms #1209

Closed

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


		}

		public Delegate[] CreateGetters(IRow input, Func<int, bool> activeOutput, out Action disposer)


		private Delegate MakeGetter<T>(IRow row, int src) => row.GetGetter<T>(src);

		public Func<int, bool> GetDependencies(Func<int, bool> activeOutput) => (col) => (activeOutput(col) && _originalColumnSources.Contains(col));

		@@ -18,6 +21,9 @@
		[assembly: LoadableClass(CopyColumnsTransform.Summary, typeof(CopyColumnsTransform), null, typeof(SignatureLoadDataTransform),

Implementing copy column estimator #706

Implementing copy column estimator #706

Conversation

Ivanidzo4ka commented Aug 21, 2018

Ivanidzo4ka Aug 21, 2018 • edited by Zruty0 Loading

Choose a reason for hiding this comment

Zruty0 Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Aug 21, 2018 • edited 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

Zruty0 Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 commented Aug 21, 2018 • edited Loading

Zruty0 Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 Aug 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zruty0 Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

Zruty0 commented Aug 23, 2018 • edited Loading

Zruty0 left a comment

Choose a reason for hiding this comment

Zruty0 left a comment

Choose a reason for hiding this comment

TomFinley Aug 24, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomFinley Aug 24, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

TomFinley Aug 24, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

TomFinley Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

TomFinley Aug 24, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

TomFinley Aug 24, 2018 • edited by Ivanidzo4ka Loading

Choose a reason for hiding this comment

TomFinley left a comment

Choose a reason for hiding this comment

Ivanidzo4ka Aug 21, 2018 •

edited by Zruty0

Loading

Zruty0 Aug 21, 2018 •

edited

Loading

Zruty0 Aug 21, 2018 •

edited

Loading

Zruty0 Aug 21, 2018 •

edited

Loading

Zruty0 Aug 21, 2018 •

edited

Loading

Zruty0 Aug 23, 2018 •

edited

Loading

Zruty0 Aug 21, 2018 •

edited

Loading

Zruty0 Aug 21, 2018 •

edited

Loading

Zruty0 Aug 21, 2018 •

edited

Loading

Zruty0 Aug 21, 2018 •

edited

Loading

Zruty0 Aug 21, 2018 •

edited

Loading

Zruty0 Aug 21, 2018 •

edited

Loading

Zruty0 Aug 21, 2018 •

edited

Loading

Zruty0 Aug 21, 2018 •

edited

Loading

Zruty0 Aug 21, 2018 •

edited

Loading

Zruty0 commented Aug 21, 2018 •

edited

Loading

Zruty0 Aug 21, 2018 •

edited

Loading

Zruty0 Aug 21, 2018 •

edited

Loading

Zruty0 Aug 22, 2018 •

edited

Loading

Zruty0 Aug 22, 2018 •

edited

Loading

Zruty0 Aug 23, 2018 •

edited

Loading

Zruty0 commented Aug 23, 2018 •

edited

Loading

TomFinley Aug 24, 2018 •

edited by Ivanidzo4ka

Loading

TomFinley Aug 24, 2018 •

edited by Ivanidzo4ka

Loading

TomFinley Aug 24, 2018 •

edited by Ivanidzo4ka

Loading

TomFinley Aug 24, 2018 •

edited

Loading

TomFinley Aug 24, 2018 •

edited by Ivanidzo4ka

Loading

TomFinley Aug 24, 2018 •

edited by Ivanidzo4ka

Loading