Towards #3204 - ColumCopying documentation #3316

sfilipi · 2019-04-12T20:07:02Z

Adhering to the template in #3204 (comment) for the ColumnCopying estimator extensions, estimator, transformer.

codecov · 2019-04-12T20:46:09Z

Codecov Report

Merging #3316 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3316      +/-   ##
==========================================
+ Coverage   72.65%   72.65%   +<.01%     
==========================================
  Files         807      807              
  Lines      145191   145191              
  Branches    16223    16223              
==========================================
+ Hits       105485   105486       +1     
  Misses      35290    35290              
+ Partials     4416     4415       -1

Flag	Coverage Δ
#Debug	`72.65% <ø> (ø)`	⬆️
#production	`68.18% <ø> (ø)`	⬆️
#test	`88.97% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
.../Microsoft.ML.Data/Transforms/ExtensionsCatalog.cs	`100% <ø> (ø)`	⬆️
src/Microsoft.ML.Data/Transforms/ColumnCopying.cs	`85.43% <ø> (ø)`	⬆️
src/Microsoft.ML.FastTree/TreeTrainersCatalog.cs	`94.18% <0%> (ø)`	⬆️
src/Microsoft.ML.Transforms/NormalizerCatalog.cs	`84.78% <0%> (ø)`	⬆️
...StandardTrainers/Standard/LinearModelParameters.cs	`60.31% <0%> (+0.26%)`	⬆️

shmoradims · 2019-04-12T21:05:59Z

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

@@ -31,8 +31,22 @@
 namespace Microsoft.ML.Transforms
 {
    /// <summary>
-    /// <see cref="ColumnCopyingEstimator"/> copies the input column to another column named as specified in the parameters of the transformation.
+    /// Estimator creating the <see cref="ColumnCopyingTransformer"/>.


Estimator [](start = 8, length = 9)

Are we doing cref=IEstimator like in trainers, or is that too much? #Closed

I prefer the wording in the trainers i.e. "The for the XXX transformer." (Not fussed about whether we cref the Estimator or not).

I think using creating here is confusing, as we create the estimator object using the extension method. #Resolved

shmoradims · 2019-04-12T21:06:22Z

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

@@ -51,11 +51,15 @@ public InputOutputColumnPair(string outputColumnName, string inputColumnName = n
    public static class TransformExtensionsCatalog
    {
        /// <summary>
-        /// Copies the input column to another column named as specified in <paramref name="outputColumnName"/>.
+        /// Creates a <see cref="ColumnCopyingEstimator"/> that copies the data from the column specified in <paramref name="inputColumnName"/>


Creates [](start = 12, length = 7)

Create #Closed

shmoradims · 2019-04-12T21:08:39Z

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

@@ -51,11 +51,15 @@ public InputOutputColumnPair(string outputColumnName, string inputColumnName = n
    public static class TransformExtensionsCatalog
    {
        /// <summary>
-        /// Copies the input column to another column named as specified in <paramref name="outputColumnName"/>.
+        /// Creates a <see cref="ColumnCopyingEstimator"/> that copies the data from the column specified in <paramref name="inputColumnName"/>


that [](start = 58, length = 5)

'that' -> ', which' ? #Closed

this that is valid, unless is a person. Then we need to use which? @natke?

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

This is one that always gets me. Strictly, "that" is used when you are defining the thing you are talking about, and "which" is used when you are giving extra information about the thing. In this case I would say we are in the second category because we know we are talking about a ColumnCopyEstimator. That is a very long-winded way of saying my vote goes with "Creates a ### , which copies the data from the column specified in " #Resolved

thanks for clearing it!

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

shmoradims · 2019-04-12T21:11:15Z

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

-        /// <param name="outputColumnName">Name of the column resulting from the transformation of <paramref name="inputColumnName"/>.</param>
-        /// <param name="inputColumnName">Name of the columns to transform.</param>
+        /// <param name="outputColumnName">Name of the column resulting from the transformation of <paramref name="inputColumnName"/>.
+        /// The type of data, is the same as the one in the input column.</param>


The type of data, [](start = 12, length = 17)

This column's data type will be the same as that of the input column? #Resolved

shmoradims · 2019-04-12T21:12:40Z

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

+        /// <param name="outputColumnName">Name of the column resulting from the transformation of <paramref name="inputColumnName"/>.
+        /// The type of data, is the same as the one in the input column.</param>
+        /// <param name="inputColumnName">Name of the column to copy the data from.
+        /// This estimator operates over any type of data.</param>


type of data [](start = 45, length = 12)

type of data -> data type?
#Closed

shmoradims · 2019-04-12T21:22:20Z

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

        /// </summary>
+        /// <remarks>For more information on this estimator see <see cref="ColumnCopyingEstimator"/>.</remarks>


on this estimator [](start = 41, length = 18)

let's just drop 'on this estimator' b/c this is the extension method. I think 'for more information see ...' is enough #Resolved

shmoradims · 2019-04-12T21:28:44Z

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

@@ -67,6 +81,10 @@ public override SchemaShape GetOutputSchema(SchemaShape inputSchema)
        }
    }

+    /// <summary>
+    /// Transformer that creates a new column, named as specified in the output column name parameters, and
+    /// copies the data from the input column to this new column.


I think ColumnCopyingEstimator should be the main page and this content should go there.

Here we can just say "The transformer resulted from fitting <cref=ColumnCopyingEstimator>" or something short that links back to the estimator.

#Resolved

I like it!

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

shmoradims · 2019-04-12T21:29:17Z

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

+    /// <remarks>
+    /// <format type="text/markdown"><![CDATA[
+    /// ### Estimator FAQ
+    /// | Question | Answer |


change similar to my PR #Resolved

shmoradims · 2019-04-12T21:30:11Z

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

+    /// | Question | Answer |
+    /// | -- | -- |
+    /// | Does this estimator need to look at the data to train its parameters? | No |
+    /// | Input column data type. | Any |


. [](start = 32, length = 1)

is the period necessary? #Resolved

if the above have a ? feels natural to have a . here. IDK, @natke?

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

I'd say no to the "." :-) #Resolved

shmoradims · 2019-04-12T21:31:56Z

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

+    /// | -- | -- |
+    /// | Does this estimator need to look at the data to train its parameters? | No |
+    /// | Input column data type. | Any |
+    /// | Output column data type. | The same as the data type in the input column. |


maybe we can add a new row for "Associated transformer"? #Closed

? you mean the transformer that gets generated from Fit?

it is right below the table.

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

shmoradims · 2019-04-12T21:32:12Z

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

+    /// | Does this estimator need to look at the data to train its parameters? | No |
+    /// | Input column data type. | Any |
+    /// | Output column data type. | The same as the data type in the input column. |
+    ///


/// [](start = 4, length = 3)

we also need 'additional nugets' here

oh, that is going even on that components that are part of Microsoft.ML NuGet? Should we put it only on the ones that are not on the main NuGet?

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

|"Required NuGet in addition to Microsoft.ML" | None|

In reply to: 275072871 [](ancestors = 275072871,275070680)

natke · 2019-04-12T22:06:59Z

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

@@ -31,8 +31,22 @@
 namespace Microsoft.ML.Transforms
 {
    /// <summary>
-    /// <see cref="ColumnCopyingEstimator"/> copies the input column to another column named as specified in the parameters of the transformation.
+    /// Estimator creating the <see cref="ColumnCopyingTransformer"/>.


I prefer the wording in the trainers i.e. "The for the XXX transformer." (Not fussed about whether we cref the Estimator or not).

I think using creating here is confusing, as we create the estimator object using the extension method. #Resolved

natke · 2019-04-12T22:09:07Z

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

+    /// | Question | Answer |
+    /// | -- | -- |
+    /// | Does this estimator need to look at the data to train its parameters? | No |
+    /// | Input column data type. | Any |


I'd say no to the "." :-) #Resolved

natke · 2019-04-12T22:19:43Z

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

@@ -51,11 +51,15 @@ public InputOutputColumnPair(string outputColumnName, string inputColumnName = n
    public static class TransformExtensionsCatalog
    {
        /// <summary>
-        /// Copies the input column to another column named as specified in <paramref name="outputColumnName"/>.
+        /// Creates a <see cref="ColumnCopyingEstimator"/> that copies the data from the column specified in <paramref name="inputColumnName"/>


This is one that always gets me. Strictly, "that" is used when you are defining the thing you are talking about, and "which" is used when you are giving extra information about the thing. In this case I would say we are in the second category because we know we are talking about a ColumnCopyEstimator. That is a very long-winded way of saying my vote goes with "Creates a ### , which copies the data from the column specified in " #Resolved

natke · 2019-04-12T22:21:51Z

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

        /// </summary>
-        /// <param name="catalog">The transform's catalog</param>
-        /// <param name="columns">The pairs of input and output columns.</param>
+        /// <remarks>This estimator can operate over several columns.


Should we say this transform? #Resolved

natke · 2019-04-12T22:22:00Z

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

-        /// <param name="catalog">The transform's catalog</param>
-        /// <param name="columns">The pairs of input and output columns.</param>
+        /// <remarks>This estimator can operate over several columns.
+        /// For more information on this estimator see <see cref="ColumnCopyingEstimator"/>.</remarks>


Drop estimator? #Resolved

natke · 2019-04-12T22:30:30Z

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

+    /// | -- | -- |
+    /// | Does this estimator need to look at the data to train its parameters? | No |
+    /// | Input column data type | Any |
+    /// | Output column data type | The same as the data type in the input column. |


Tiny nitpick: remove the "." #Resolved

natke · 2019-04-12T22:31:30Z

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

@@ -51,11 +51,15 @@ public InputOutputColumnPair(string outputColumnName, string inputColumnName = n
    public static class TransformExtensionsCatalog
    {
        /// <summary>
-        /// Copies the input column to another column named as specified in <paramref name="outputColumnName"/>.
+        /// Create a <see cref="ColumnCopyingEstimator"/> which copies the data from the column specified in <paramref name="inputColumnName"/>


Another nit: add "," before which #Resolved

natke · 2019-04-12T22:33:35Z

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

@@ -67,11 +71,13 @@ public static ColumnCopyingEstimator CopyColumns(this TransformsCatalog catalog,
            => new ColumnCopyingEstimator(CatalogUtils.GetEnvironment(catalog), outputColumnName, inputColumnName);

        /// <summary>
-        /// Copies the input column, name specified in the first item of the tuple,
-        /// to another column, named as specified in the second item of the tuple.
+        /// Create a <see cref="ColumnCopyingEstimator"/> which copies the data from the column specified in <see cref="InputOutputColumnPair.InputColumnName" />


nit: add "," before which #Resolved

wschin · 2019-04-15T16:43:01Z

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

+    /// copies the data from the input column to this new column.
+    /// ]]>
+    /// </format>
+    /// </remarks>


Could you describe at least one way to use this transform? Maybe add a link to ../docs/samples/docs/samples/Microsoft.ML.Samples/Dynamic/Transforms/CopyColumns.cs like what we will do to trainers? If a user is reading this document, they want to use it in their pipeline. #Resolved

wschin · 2019-04-15T16:43:37Z

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

@@ -51,11 +51,15 @@ public InputOutputColumnPair(string outputColumnName, string inputColumnName = n
    public static class TransformExtensionsCatalog
    {
        /// <summary>
-        /// Copies the input column to another column named as specified in <paramref name="outputColumnName"/>.
+        /// Create a <see cref="ColumnCopyingEstimator"/>, which copies the data from the column specified in <paramref name="inputColumnName"/>
+        /// to a new column: <paramref name="outputColumnName"/>.


Maybe

Suggested change

/// to a new column: <paramref name="outputColumnName"/>.

/// to a new column <paramref name="outputColumnName"/>.

``` #Pending

Just remove the:? I think i might leave it, since Natalie seems ok with it?

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

wschin

LGTM. My only wish is to add example link to estimator.

…usages.

shmoradims · 2019-04-15T18:25:34Z

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

        /// </summary>
+        /// <remarks>For more information see <see cref="ColumnCopyingEstimator"/>.</remarks>


/// For more information see . [](start = 8, length = 85)

we don't have this for trainers. we should be consistent. is this helpful?
@natke what's your take?

We do already link to ColumnCopyingEstimator above, but this doesn't hurt @shmoradims

shmoradims

natke · 2019-04-15T19:14:48Z

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

        /// </summary>
+        /// <remarks>For more information see <see cref="ColumnCopyingEstimator"/>.</remarks>


We do already link to ColumnCopyingEstimator above, but this doesn't hurt @shmoradims

* applying the dotnet#3204 template to ColumnCopying.

sfilipi added 2 commits April 12, 2019 10:49

applying the dotnet#3204 template to columnCopying.

1d924ae

Adding column data type information.

b3d0e64

sfilipi requested review from artidoro, shmoradims, natke and zeahmed and removed request for artidoro and zeahmed April 12, 2019 20:07

shmoradims reviewed Apr 12, 2019

View reviewed changes

Addressing most of Shahab's comments.

b38f0fb

natke suggested changes Apr 12, 2019

View reviewed changes

Addressing Natalie's comments

0abeb92

natke approved these changes Apr 12, 2019

View reviewed changes

punctuation.

42e2cda

natke approved these changes Apr 12, 2019

View reviewed changes

sfilipi self-assigned this Apr 13, 2019

sfilipi requested review from rogancarr and zeahmed April 15, 2019 16:38

wschin reviewed Apr 15, 2019

View reviewed changes

wschin approved these changes Apr 15, 2019

View reviewed changes

Adding the seealso section referencing to the extensions with sample …

ff7fa6f

…usages.

shmoradims reviewed Apr 15, 2019

View reviewed changes

shmoradims approved these changes Apr 15, 2019

View reviewed changes

natke approved these changes Apr 15, 2019

View reviewed changes

Shahab's comment.

421069d

sfilipi merged commit 64badcf into dotnet:master Apr 15, 2019

sfilipi deleted the 3204Simple branch April 15, 2019 21:09

sfilipi added a commit to sfilipi/machinelearning-1 that referenced this pull request Apr 16, 2019

Towards dotnet#3204 - ColumCopying documentation (dotnet#3316)

dbf017a

* applying the dotnet#3204 template to ColumnCopying.

sfilipi mentioned this pull request Apr 16, 2019

CherryPicking #3316 - ColumCopying documentation #3348

Closed

najeeb-kazmi mentioned this pull request Apr 18, 2019

Towards #3204 - Documentation for MLContext.Transforms.Categorical #3388

Merged

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

		/// </summary>
		/// <remarks>For more information on this estimator see <see cref="ColumnCopyingEstimator"/>.</remarks>

	/// to a new column: <paramref name="outputColumnName"/>.
	/// to a new column <paramref name="outputColumnName"/>.
	``` #Pending

		/// </summary>
		/// <remarks>For more information see <see cref="ColumnCopyingEstimator"/>.</remarks>

Towards #3204 - ColumCopying documentation #3316

Towards #3204 - ColumCopying documentation #3316

Conversation

sfilipi commented Apr 12, 2019

codecov bot commented Apr 12, 2019 • edited Loading

Codecov Report

shmoradims Apr 12, 2019 • edited Loading

Choose a reason for hiding this comment

natke Apr 12, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

shmoradims Apr 12, 2019 • edited Loading

Choose a reason for hiding this comment

shmoradims Apr 12, 2019 • edited Loading

Choose a reason for hiding this comment

sfilipi Apr 12, 2019

Choose a reason for hiding this comment

natke Apr 12, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

sfilipi Apr 12, 2019

Choose a reason for hiding this comment

shmoradims Apr 12, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

shmoradims Apr 12, 2019 • edited Loading

Choose a reason for hiding this comment

shmoradims Apr 12, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

shmoradims Apr 12, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

sfilipi Apr 12, 2019

Choose a reason for hiding this comment

shmoradims Apr 12, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

shmoradims Apr 12, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

sfilipi Apr 12, 2019

Choose a reason for hiding this comment

natke Apr 12, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

shmoradims Apr 12, 2019 • edited Loading

Choose a reason for hiding this comment

sfilipi Apr 12, 2019

Choose a reason for hiding this comment

shmoradims Apr 12, 2019 • edited Loading

Choose a reason for hiding this comment

sfilipi Apr 12, 2019 • edited Loading

Choose a reason for hiding this comment

shmoradims Apr 15, 2019

Choose a reason for hiding this comment

natke Apr 12, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

natke Apr 12, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

natke Apr 12, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

natke Apr 12, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

natke Apr 12, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

natke Apr 12, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

natke Apr 12, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

natke Apr 12, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

wschin Apr 15, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

wschin Apr 15, 2019 • edited by sfilipi Loading

Choose a reason for hiding this comment

sfilipi Apr 15, 2019

Choose a reason for hiding this comment

wschin left a comment • edited Loading

Choose a reason for hiding this comment

shmoradims Apr 15, 2019 • edited Loading

Choose a reason for hiding this comment

natke Apr 15, 2019 • edited Loading

Choose a reason for hiding this comment

shmoradims left a comment

Choose a reason for hiding this comment

natke Apr 15, 2019 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Apr 12, 2019 •

edited

Loading

shmoradims Apr 12, 2019 •

edited

Loading

natke Apr 12, 2019 •

edited by sfilipi

Loading

shmoradims Apr 12, 2019 •

edited

Loading

shmoradims Apr 12, 2019 •

edited

Loading

natke Apr 12, 2019 •

edited by sfilipi

Loading

shmoradims Apr 12, 2019 •

edited by sfilipi

Loading

shmoradims Apr 12, 2019 •

edited

Loading

shmoradims Apr 12, 2019 •

edited by sfilipi

Loading

shmoradims Apr 12, 2019 •

edited by sfilipi

Loading

shmoradims Apr 12, 2019 •

edited by sfilipi

Loading

shmoradims Apr 12, 2019 •

edited by sfilipi

Loading

natke Apr 12, 2019 •

edited by sfilipi

Loading

shmoradims Apr 12, 2019 •

edited

Loading

shmoradims Apr 12, 2019 •

edited

Loading

sfilipi Apr 12, 2019 •

edited

Loading

natke Apr 12, 2019 •

edited by sfilipi

Loading

natke Apr 12, 2019 •

edited by sfilipi

Loading

natke Apr 12, 2019 •

edited by sfilipi

Loading

natke Apr 12, 2019 •

edited by sfilipi

Loading

natke Apr 12, 2019 •

edited by sfilipi

Loading

natke Apr 12, 2019 •

edited by sfilipi

Loading

natke Apr 12, 2019 •

edited by sfilipi

Loading

natke Apr 12, 2019 •

edited by sfilipi

Loading

wschin Apr 15, 2019 •

edited by sfilipi

Loading

wschin Apr 15, 2019 •

edited by sfilipi

Loading

wschin left a comment •

edited

Loading

shmoradims Apr 15, 2019 •

edited

Loading

natke Apr 15, 2019 •

edited

Loading

natke Apr 15, 2019 •

edited

Loading