[Part 3] Added convenience constructors for set of transforms. #520

zeahmed · 2018-07-10T20:24:59Z

This PR fixes #518. The convenience constructors were added for following transforms.

GroupTransform.cs
HashJoinTransform.cs
KeyToBinaryVectorTransform.cs
LoadTransform.cs
MissingValueIndicatorTransform.cs
MutualInformationFeatureSelectionTransform.cs
NADropTransform.cs
NAHandleTransform.cs
NAIndicatorTransform.cs
NAReplaceTransform.cs
OptionalColumnTransform.cs
RffTransform.cs
UngroupTransform.cs
WhiteningTransform.cs

TomFinley · 2018-07-12T05:09:48Z

src/Microsoft.ML.Transforms/UngroupTransform.cs

@@ -79,6 +79,18 @@ public sealed class Arguments : TransformInputBase

        private readonly SchemaImpl _schemaImpl;

+        /// <summary>
+        /// Convenience constructor for public facing API.


Convenience constructor for public facing API. [](start = 12, length = 46)

We might want to improve this description, since from the point of view of the user it doesn't really help them much to know that they're using the public facing API really. #Closed

Ah... I see this was mostly done via copy-paste, I'm afraid that an actual description will be necessary. Look on the bright side, I think that an understanding of how it could be used can only help you imagine the best way in which the convenience should be structured.

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

Probably obvious, but just in case not clear this is intended as a general comment.

In reply to: 201912669 [](ancestors = 201912669,201910848)

I was also thinking about adding more description to the constructors and wanted to open a separate issue to include clear description to each of the convenience constructors. What do you say?

In reply to: 201912748 [](ancestors = 201912748,201912669,201910848)

Opened issue #524 for addressing this comment. Because it needs to be done for all the transforms we have worked so far.

In reply to: 202117587 [](ancestors = 202117587,201912748,201912669,201910848)

Fair enough, sounds good @zeahmed

In reply to: 202121318 [](ancestors = 202121318,202117587,201912748,201912669,201910848)

TomFinley · 2018-07-12T05:11:50Z

src/Microsoft.ML.Transforms/UngroupTransform.cs

+        /// Convenience constructor for public facing API.
+        /// </summary>
+        /// <param name="env">Host Environment.</param>
+        /// <param name="input">Input <see cref="IDataView"/>. This is the output from previous transform or loader.</param>


Input . This is the output from previous transform or loader [](start = 32, length = 83)

This on the other hand might be over-description. Certainly this is not the best place for people to learn how to instantiate IDataViews if they don't already know how to do so. in It's also not quite correct, since there are certainly other data views other than loaders and transforms. #Closed

ok, I will update it as a part of this issue #524.

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

TomFinley · 2018-07-12T05:12:45Z

Descriptions of these might be good.

I see that the summary comment on the class itself already has some documentation. The relevant section where it explains the enum should be moved, and formatted appropriately, etc. #Closed

Refers to: src/Microsoft.ML.Transforms/UngroupTransform.cs:69 in d9c4a29. [](commit_id = d9c4a29, deletion_comment = False)

TomFinley · 2018-07-12T05:13:33Z

src/Microsoft.ML.Transforms/RffTransform.cs

+        /// <param name="input">Input <see cref="IDataView"/>. This is the output from previous transform or loader.</param>
+        /// <param name="name">Name of the output column.</param>
+        /// <param name="source">Name of the column to be transformed. If this is null '<paramref name="name"/>' will be used.</param>
+        /// <param name="newDim">The number of random Fourier features to create.</param>


/// The number of random Fourier features to create. [](start = 8, length = 81)

This ought to be required. #Closed

TomFinley · 2018-07-12T05:14:38Z

src/Microsoft.ML.Transforms/RffTransform.cs

+        /// <param name="name">Name of the output column.</param>
+        /// <param name="source">Name of the column to be transformed. If this is null '<paramref name="name"/>' will be used.</param>
+        /// <param name="newDim">The number of random Fourier features to create.</param>
+        /// <param name="useSin">create two features for every random Fourier frequency? (one for cos and one for sin).</param>


create two features for every random Fourier frequency? [](start = 33, length = 55)

So, if newDim was 10, and I set this to true, does the output become 20 or something?

This guy is a little complex. I wonder if we can get away with not including it in the convenience? #Closed

Yes, it creates 2 * newDim features when useSin=true. Yes, we can remove useSin parameter as it's default value is also false.

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

TomFinley · 2018-07-12T05:15:38Z

"", "ProduceIdTransform", "ProduceId")]

My thinking is that transforms that are hidden probably don't need convenience constructors. This thing in particular is mostly something I have for debugging. #Closed

Refers to: src/Microsoft.ML.Transforms/ProduceIdTransform.cs:12 in d9c4a29. [](commit_id = d9c4a29, deletion_comment = False)

TomFinley · 2018-07-12T05:17:24Z

public class OptionalColumnTransform : RowToRowMapperTransformBase

I'm not sure what this class is, ad why it isn't sealed. Do you understand it? #Closed

Refers to: src/Microsoft.ML.Transforms/OptionalColumnTransform.cs:29 in d9c4a29. [](commit_id = d9c4a29, deletion_comment = False)

TomFinley · 2018-07-12T05:18:58Z

        Maximum,

Now that these are being used as programmatic constructs maybe migrate the documentation above. #Closed

Refers to: src/Microsoft.ML.Transforms/NAHandleTransform.cs:43 in d9c4a29. [](commit_id = d9c4a29, deletion_comment = False)

TomFinley · 2018-07-12T05:19:36Z

src/Microsoft.ML.Transforms/MutualInformationFeatureSelection.cs

        }

        internal static string RegistrationName = "MutualInformationFeatureSelectionTransform";

+        /// <summary>
+        /// A helper method to create <see cref="MutualInformationFeatureSelectionTransform"/> for public facing API.


MutualInformationFeatureSelectionTransform [](start = 49, length = 42)

You can't be creating it, it's a static class. #Closed

Similar comments as before, when documenting we ought to describe what it is actually useful for, not just describing its return values (which are part of the method signature, and we aren't even doing that quite right).

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

I will add detailed comments as part of #524.

In reply to: 201912123 [](ancestors = 201912123,201912038)

TomFinley · 2018-07-12T05:21:18Z

"", "MissingValueIndicatorTransform", "MissingValueTransform", "MissingTransform", "Missing")]

Same, this is hidden, and mostly deprecated in favor of the NAIndicatorTransform. We only really have it around for backwards compatibility reasons... #Closed

Refers to: src/Microsoft.ML.Transforms/MissingValueIndicatorTransform.cs:16 in d9c4a29. [](commit_id = d9c4a29, deletion_comment = False)

TomFinley · 2018-07-12T05:22:35Z

/// Tom  [Table, Kitten]

Oh, memories. :D #Closed

Refers to: src/Microsoft.ML.Transforms/GroupTransform.cs:50 in d9c4a29. [](commit_id = d9c4a29, deletion_comment = False)

TomFinley · 2018-07-12T05:23:53Z

src/Microsoft.ML.Transforms/GroupTransform.cs

+        /// <param name="input">Input <see cref="IDataView"/>. This is the output from previous transform or loader.</param>
+        /// <param name="groupKey">Columns to group by</param>
+        /// <param name="columns">Columns to group together</param>
+        public GroupTransform(IHostEnvironment env, IDataView input, string[] groupKey, params string[] columns)


string[] groupKey [](start = 69, length = 17)

Considering the cases I'm aware of where it is used, I have to think that a single string would be preferable for the convenience constructor. #Closed

zeahmed · 2018-07-12T18:44:33Z

public class OptionalColumnTransform : RowToRowMapperTransformBase

After a long discussion yesterday on a separate thread I got to know the purpose of this transform.

This is the transform used to mark some of the columns (e.g. Label) optional during training. So that the columns is not required during scoring. At scoring time, TLC checks to see if the data schema for scoring matches the data used for training except for the optional columns.

I am not sure why its not sealed. Looking at all other transforms, I assume it should be sealed as well. I am not changing it right now unless you see it feasible to do so.

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

Refers to: src/Microsoft.ML.Transforms/OptionalColumnTransform.cs:29 in d9c4a29. [](commit_id = d9c4a29, deletion_comment = False)

zeahmed · 2018-07-12T18:59:28Z

/// Tom  [Table, Kitten]

hahaha...easy to catch culprits this way...:D

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

Refers to: src/Microsoft.ML.Transforms/GroupTransform.cs:50 in d9c4a29. [](commit_id = d9c4a29, deletion_comment = False)

TomFinley · 2018-07-13T17:51:20Z

src/Microsoft.ML.Transforms/ProduceIdTransform.cs

+        private static class Defaults
+        {
+            public const string Column = "Id";
+        }


I think we probably don't need this Defaults class any longer. #Closed

Indeed we might as well revert the whole file probably.

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

TomFinley · 2018-07-13T17:52:30Z

src/Microsoft.ML.Transforms/NAHandleTransform.cs

@@ -34,12 +34,30 @@ namespace Microsoft.ML.Runtime.Data
    /// </summary>
    public static class NAHandleTransform
    {
+        /// <summary>
+        /// 
+        /// </summary>
        public enum ReplacementKind


Empty? #Closed

TomFinley · 2018-07-13T17:53:51Z

public class OptionalColumnTransform : RowToRowMapperTransformBase

May as well. And since you've gone to the effort of digging up what it's actually useful for maybe just paste that into an XML comment.

In reply to: 404611599 [](ancestors = 404611599,404392058)

Refers to: src/Microsoft.ML.Transforms/OptionalColumnTransform.cs:29 in d9c4a29. [](commit_id = d9c4a29, deletion_comment = False)

TomFinley · 2018-07-13T17:54:21Z

src/Microsoft.ML.Transforms/UngroupTransform.cs

        public enum UngroupMode
        {
+            /// <summary>
+            /// A number of output rows are equal to the minimum length of pivot columns


All these "A number" would read better as "The number". #Closed

TomFinley · 2018-07-13T17:54:56Z

Getting really close thanks @zeahmed! #Closed

TomFinley · 2018-07-13T21:08:39Z

src/Microsoft.ML.Transforms/OptionalColumnTransform.cs

+    /// <summary>
+    /// This transform is used to mark some of the columns (e.g. Label) optional during training So that the columns is not required during scoring..
+    /// At scoring time, it is checked if the data schema for scoring matches the data used for training except for the optional columns.
+    /// </summary>
    public class OptionalColumnTransform : RowToRowMapperTransformBase


public class [](start = 4, length = 12)

Sealed? :) #Closed

TomFinley · 2018-07-13T21:09:11Z

src/Microsoft.ML.Transforms/UngroupTransform.cs

            Outer,
+
+            /// <summary>
+            /// The number of output rows are equal to the length of the first pivot column.


are [](start = 42, length = 3)

is equal #Closed

TomFinley · 2018-07-13T21:09:16Z

src/Microsoft.ML.Transforms/UngroupTransform.cs

            Inner,
+
+            /// <summary>
+            /// The number of output rows are equal to the maximum length of pivot columns


are [](start = 42, length = 3)

is equal #Closed

TomFinley · 2018-07-13T21:09:22Z

src/Microsoft.ML.Transforms/UngroupTransform.cs

        public enum UngroupMode
        {
+            /// <summary>
+            /// The number of output rows are equal to the minimum length of pivot columns


are [](start = 42, length = 3)

is equal #Closed

TomFinley · 2018-07-13T21:13:11Z

src/Microsoft.ML.Transforms/OptionalColumnTransform.cs

@@ -26,6 +26,10 @@

 namespace Microsoft.ML.Runtime.DataPipe
 {
+    /// <summary>
+    /// This transform is used to mark some of the columns (e.g. Label) optional during training So that the columns is not required during scoring..


So [](start = 97, length = 2)

lower case "so" since this is in the middle of a sentence? #Closed

TomFinley · 2018-07-13T21:13:32Z

src/Microsoft.ML.Transforms/OptionalColumnTransform.cs

@@ -26,6 +26,10 @@

 namespace Microsoft.ML.Runtime.DataPipe
 {
+    /// <summary>
+    /// This transform is used to mark some of the columns (e.g. Label) optional during training So that the columns is not required during scoring..


columns is [](start = 109, length = 10)

"columns is" => "columns are" #Closed

TomFinley · 2018-07-13T21:13:42Z

src/Microsoft.ML.Transforms/OptionalColumnTransform.cs

@@ -26,6 +26,10 @@

 namespace Microsoft.ML.Runtime.DataPipe
 {
+    /// <summary>
+    /// This transform is used to mark some of the columns (e.g. Label) optional during training So that the columns is not required during scoring..


.. [](start = 147, length = 2)

One period instead of two. #Closed

TomFinley · 2018-07-13T21:23:09Z

src/Microsoft.ML.Transforms/OptionalColumnTransform.cs

@@ -26,6 +26,10 @@

 namespace Microsoft.ML.Runtime.DataPipe
 {
+    /// <summary>
+    /// This transform is used to mark some of the columns (e.g. Label) optional during training So that the columns is not required during scoring..
+    /// At scoring time, it is checked if the data schema for scoring matches the data used for training except for the optional columns.


At scoring time, it is checked if the data schema for scoring matches the data used for training except for the optional columns. [](start = 8, length = 129)

I'm not sure I actually understand this. What it really does is, if the column is not there, it will make it appear as if it is there with metadata filled in, and if the column is requested will return entirely default values. Or so I understand the code in the cursor. Is there some way to describe it in a way that communicates that? I'm not sure this sentence here actually communicates that. #Closed

TomFinley · 2018-07-13T23:02:49Z

src/Microsoft.ML.Transforms/OptionalColumnTransform.cs

-    public class OptionalColumnTransform : RowToRowMapperTransformBase
+    /// <summary>
+    /// This transform is used to mark some of the columns (e.g. Label) optional during training so that the columns are not required during scoring.
+    /// When applied to new data, if optional columns are not present a meta column is created having the same properties (e.g. 'name', 'type' etc.) as used during training.


meta column [](start = 72, length = 11)

The phrase meta-column is a bit odd, I'm not sure that's a good description. Maybe dummy column? Mock column? #Closed

TomFinley · 2018-07-13T23:04:04Z

src/Microsoft.ML.Transforms/OptionalColumnTransform.cs

+    ///     - scalar for scalar column
+    ///     - totally sparse vector for vector column. 
+    /// If value of the column is requested the default value will be returned.
+    /// </summary>


This last line seems redundant. #Closed

TomFinley

codemzs

zeahmed · 2018-07-16T18:51:00Z

Thanks @TomFinley and @codemzs!

…t#520)

[Part 3] Added convenience constructors for set of transforms.

d9c4a29

zeahmed requested review from codemzs, Ivanidzo4ka and TomFinley July 10, 2018 20:25

TomFinley reviewed Jul 12, 2018

View reviewed changes

zeahmed mentioned this pull request Jul 12, 2018

Add description to convenience constructors. #524

Closed

Addressed reviewers' comments.

6cd77eb

TomFinley reviewed Jul 13, 2018

View reviewed changes

Addressed reviewers' comments.

75a8b6d

TomFinley reviewed Jul 13, 2018

View reviewed changes

Addressed reviewers' comments.

d72a4ac

TomFinley reviewed Jul 13, 2018

View reviewed changes

zeahmed added 2 commits July 13, 2018 16:09

Updated OptionalColumnTransform comments.

3dd9847

Updated comments.

730fe93

TomFinley approved these changes Jul 13, 2018

View reviewed changes

codemzs approved these changes Jul 16, 2018

View reviewed changes

zeahmed merged commit c491651 into dotnet:master Jul 16, 2018

zeahmed deleted the convenience_constructor3 branch July 17, 2018 17:42

eerhardt pushed a commit to eerhardt/machinelearning that referenced this pull request Jul 27, 2018

[Part 3] Added convenience constructors for set of transforms. (dotne…

df33732

…t#520)

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

[Part 3] Added convenience constructors for set of transforms. #520

[Part 3] Added convenience constructors for set of transforms. #520

Uh oh!

Conversation

zeahmed commented Jul 10, 2018

Uh oh!

TomFinley Jul 12, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeahmed Jul 12, 2018 • 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!

zeahmed Jul 12, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

TomFinley Jul 12, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

TomFinley commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TomFinley Jul 12, 2018 • 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!

TomFinley Jul 12, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

TomFinley commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TomFinley commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TomFinley commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TomFinley Jul 12, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomFinley commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TomFinley commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TomFinley Jul 12, 2018 • 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!

zeahmed commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zeahmed commented Jul 12, 2018

Uh oh!

TomFinley Jul 13, 2018 • 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!

Choose a reason for hiding this comment

TomFinley Jul 12, 2018 •

edited

Loading

zeahmed Jul 12, 2018 •

edited

Loading

zeahmed Jul 12, 2018 •

edited

Loading

TomFinley Jul 12, 2018 •

edited

Loading

TomFinley commented Jul 12, 2018 •

edited

Loading

TomFinley Jul 12, 2018 •

edited

Loading

TomFinley Jul 12, 2018 •

edited

Loading

TomFinley commented Jul 12, 2018 •

edited

Loading

TomFinley commented Jul 12, 2018 •

edited

Loading

TomFinley commented Jul 12, 2018 •

edited

Loading

TomFinley Jul 12, 2018 •

edited

Loading

TomFinley commented Jul 12, 2018 •

edited

Loading

TomFinley commented Jul 12, 2018 •

edited

Loading

TomFinley Jul 12, 2018 •

edited

Loading

zeahmed commented Jul 12, 2018 •

edited

Loading

TomFinley Jul 13, 2018 •

edited

Loading

TomFinley Jul 13, 2018 •

edited

Loading

TomFinley Jul 13, 2018 •

edited

Loading

TomFinley commented Jul 13, 2018 •

edited by zeahmed

Loading

TomFinley Jul 13, 2018 •

edited

Loading

TomFinley Jul 13, 2018 •

edited

Loading

TomFinley Jul 13, 2018 •

edited

Loading

TomFinley Jul 13, 2018 •

edited

Loading

TomFinley Jul 13, 2018 •

edited

Loading

TomFinley Jul 13, 2018 •

edited

Loading

TomFinley Jul 13, 2018 •

edited

Loading

TomFinley Jul 13, 2018 •

edited

Loading

TomFinley Jul 13, 2018 •

edited

Loading

TomFinley Jul 13, 2018 •

edited

Loading