Updating the CopyColumnsEstimator and Transform to use common code (#706) #1141

singlis · 2018-10-03T21:07:35Z

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

(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

dnfclas · 2018-10-03T21:07:46Z

All CLA requirements met.

Ivanidzo4ka · 2018-10-04T02:45:11Z

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

-[assembly: LoadableClass(CopyColumnsTransform.Summary, typeof(CopyColumnsRowMapper), null, typeof(SignatureLoadRowMapper),
-    CopyColumnsTransform.UserName, CopyColumnsRowMapper.LoaderSignature)]
+[assembly: LoadableClass(typeof(IRowMapper), typeof(CopyColumnsTransform), null, typeof(SignatureLoadRowMapper),
+    CopyColumnsTransform.UserName, CopyColumnsTransform.LoaderSignature)]

 namespace Microsoft.ML.Runtime.Data


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

can you change it to Microsoft.ML.Transforms? #Closed

The namespace also reflects the folder location. If we change the namespace, should this also move into Microsoft.ML.Transforms?

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

Actually I see some Transforms in Microsoft.ML.Transform that have the namespace Microsoft.ML.Runtime.Data - so I am confused on this.

In reply to: 222767195 [](ancestors = 222767195,222523600)

Ivanidzo4ka · 2018-10-04T02:47:10Z

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

+        /// <summary>
+        /// Factory method to create from arguments
+        /// </summary>
+        public static IDataTransform Create(IHostEnvironment env, Arguments args, IDataView input)


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

Why it become public? #Closed

This remained public, please see the code in master:

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

Line 167 in 20761a3

public static IDataTransform Create(IHostEnvironment env, Arguments args, IDataView input)

Its the diff that makes it look like it transitioned from private to public.

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

Ivanidzo4ka · 2018-10-04T02:47:50Z

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

        }

-        // Factory method for SignatureLoadModel.


// Factory method for SignatureLoadModel. [](start = 7, length = 42)

I would prefer this format, in all other places we have this pattern instead of //summary #Closed

Ivanidzo4ka · 2018-10-04T02:48:34Z

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

-            //   string: input column name
-            ctx.Writer.Write(_columns.Length);
-            foreach (var column in _columns)
+            //   string: output (Name) column name


// string: output (Name) column name [](start = 12, length = 38)

I think we have SaveBaseColumns or something like that in base class. #Closed

Ivanidzo4ka · 2018-10-04T02:51:00Z

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

                var type = input.Schema.GetColumnType(colIndex);
-                result[i] = Utils.MarshalInvoke(MakeGetter<int>, type.RawType, input, colIndex);
+                return Utils.MarshalInvoke(MakeGetter<int>, type.RawType,  input, colIndex);


[](start = 73, length = 2)

Ctrl +K+D please #Closed

sfilipi · 2018-10-04T04:01:28Z

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

    {
-        private readonly (string Source, string Name)[] _columns;
-        private readonly IHost _host;
        public const string LoaderSignature = "CopyTransform";
        private const string RegistrationName = "CopyColumns";
        public const string Summary = "Copy a source column to a new column.";
        public const string UserName = "Copy Columns Transform";
        public const string ShortName = "Copy";


can the public members be made internal? if they are public, they will display in the documentation site. #Resolved

these are now internal

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

sfilipi · 2018-10-04T04:03:27Z

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

-            var length = ctx.Reader.ReadInt32();
-            var columns = new (string Source, string Name)[length];
-            for (int i = 0; i < length; i++)
+            public Mapper(CopyColumnsTransform parent, ISchema inputSchema, (string Source, string Name)[] columns)


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

would internal suffice? #Resolved

at this point I'm curious how many wrong comments can I leave here. :)
Technically it's a private class, so no one would see it any way....

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

Mapper is a private class and a subclass to CopyColumnsTransform. I went ahead and make the constructor internal. @[email protected] let me know if you have any additional feedback.

In reply to: 222533129 [](ancestors = 222533129,222532234)

I don't have any other feedback. So feel free to push your changes into PR, if that's the reason why you haven't done it yet :)

In reply to: 222782703 [](ancestors = 222782703,222533129,222532234)

sfilipi · 2018-10-04T04:05:07Z

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

-            ctx.CheckAtModel();
-            ctx.SetVersionInfo(GetVersionInfo());
+            private Delegate MakeGetter<T>(IRow input, int iinfo)
+                => input.GetGetter<T>(iinfo);


declare the delegate inside the MakeGetter above, if it is the only thing that calls it. #Resolved

sfilipi

updated namespace usage and entrypoints due to this change. - Save now uses the SaveColumns from the base class - Other various changes based upon feedback.

Ivanidzo4ka

singlis self-assigned this Oct 3, 2018

singlis requested a review from Ivanidzo4ka October 3, 2018 21:37

Ivanidzo4ka reviewed Oct 4, 2018

View reviewed changes

Ivanidzo4ka requested review from sfilipi and Zruty0 October 4, 2018 02:51

sfilipi reviewed Oct 4, 2018

View reviewed changes

sfilipi approved these changes Oct 4, 2018

View reviewed changes

shauheen added this to the 1018 milestone Oct 4, 2018

singlis added 2 commits October 4, 2018 10:59

Merge remote-tracking branch 'origin/master' into singlis/CopyColumns

cbaf276

- Moved CopyColumnsTransform into Microsoft.ML.Transforms namespace,

67dca3e

updated namespace usage and entrypoints due to this change. - Save now uses the SaveColumns from the base class - Other various changes based upon feedback.

Ivanidzo4ka approved these changes Oct 4, 2018

View reviewed changes

singlis merged commit ce7d3f6 into dotnet:master Oct 4, 2018

singlis deleted the singlis/CopyColumns branch March 7, 2019 00:54

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating the CopyColumnsEstimator and Transform to use common code (#706) #1141

Updating the CopyColumnsEstimator and Transform to use common code (#706) #1141

singlis commented Oct 3, 2018

dnfclas commented Oct 3, 2018 •

edited

Loading

Ivanidzo4ka Oct 4, 2018 •

edited

Loading

singlis Oct 4, 2018

singlis Oct 4, 2018 •

edited

Loading

Ivanidzo4ka Oct 4, 2018 •

edited

Loading

singlis Oct 4, 2018

Ivanidzo4ka Oct 4, 2018 •

edited

Loading

Ivanidzo4ka Oct 4, 2018 •

edited

Loading

Ivanidzo4ka Oct 4, 2018 •

edited

Loading

sfilipi Oct 4, 2018 •

edited by singlis

Loading

singlis Oct 4, 2018

sfilipi Oct 4, 2018 •

edited by singlis

Loading

Ivanidzo4ka Oct 4, 2018

singlis Oct 4, 2018

Ivanidzo4ka Oct 4, 2018

sfilipi Oct 4, 2018 •

edited by singlis

Loading

sfilipi left a comment

Ivanidzo4ka left a comment

Updating the CopyColumnsEstimator and Transform to use common code (#706) #1141

Updating the CopyColumnsEstimator and Transform to use common code (#706) #1141

Conversation

singlis commented Oct 3, 2018

dnfclas commented Oct 3, 2018 • edited Loading

Ivanidzo4ka Oct 4, 2018 • edited Loading

Choose a reason for hiding this comment

singlis Oct 4, 2018

Choose a reason for hiding this comment

singlis Oct 4, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Oct 4, 2018 • edited Loading

Choose a reason for hiding this comment

singlis Oct 4, 2018

Choose a reason for hiding this comment

Ivanidzo4ka Oct 4, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Oct 4, 2018 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Oct 4, 2018 • edited Loading

Choose a reason for hiding this comment

sfilipi Oct 4, 2018 • edited by singlis Loading

Choose a reason for hiding this comment

singlis Oct 4, 2018

Choose a reason for hiding this comment

sfilipi Oct 4, 2018 • edited by singlis Loading

Choose a reason for hiding this comment

Ivanidzo4ka Oct 4, 2018

Choose a reason for hiding this comment

singlis Oct 4, 2018

Choose a reason for hiding this comment

Ivanidzo4ka Oct 4, 2018

Choose a reason for hiding this comment

sfilipi Oct 4, 2018 • edited by singlis Loading

Choose a reason for hiding this comment

sfilipi left a comment

Choose a reason for hiding this comment

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

dnfclas commented Oct 3, 2018 •

edited

Loading

Ivanidzo4ka Oct 4, 2018 •

edited

Loading

singlis Oct 4, 2018 •

edited

Loading

Ivanidzo4ka Oct 4, 2018 •

edited

Loading

Ivanidzo4ka Oct 4, 2018 •

edited

Loading

Ivanidzo4ka Oct 4, 2018 •

edited

Loading

Ivanidzo4ka Oct 4, 2018 •

edited

Loading

sfilipi Oct 4, 2018 •

edited by singlis

Loading

sfilipi Oct 4, 2018 •

edited by singlis

Loading

sfilipi Oct 4, 2018 •

edited by singlis

Loading