Skip to content

Towards #3204 - ColumCopying documentation #3316

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Apr 15, 2019
Merged

Towards #3204 - ColumCopying documentation #3316

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1d924ae
applying the #3204 template to columnCopying.
sfilipi Apr 12, 2019
b3d0e64
Adding column data type information.
sfilipi Apr 12, 2019
b38f0fb
Addressing most of Shahab's comments.
sfilipi Apr 12, 2019
0abeb92
Addressing Natalie's comments
sfilipi Apr 12, 2019
42e2cda
punctuation.
sfilipi Apr 12, 2019
ff7fa6f
Adding the seealso section referencing to the extensions with sample …
sfilipi Apr 15, 2019
421069d
Shahab's comment.
sfilipi Apr 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion src/Microsoft.ML.Data/Transforms/ColumnCopying.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,23 @@
namespace Microsoft.ML.Transforms
{
/// <summary>
/// <see cref="ColumnCopyingEstimator"/> copies the input column to another column named as specified in the parameters of the transformation.
/// <see cref="IEstimator{TTransformer}"/> for the <see cref="ColumnCopyingTransformer"/>.
/// </summary>
/// <remarks>
/// <format type="text/markdown"><![CDATA[
///
/// ### Estimator Characteristics
/// | | |
/// | -- | -- |
/// | 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. |
Copy link
Contributor

@natke natke Apr 12, 2019 •
edited by sfilipi
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tiny nitpick: remove the "." #Resolved

///
Copy link

@shmoradims shmoradims Apr 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

we also need 'additional nugets' here

Copy link
Member Author

@sfilipi sfilipi Apr 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

/// The resulting <see cref="ColumnCopyingTransformer"/> 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.
/// ]]>
/// </format>
/// </remarks>
Copy link
Member

@wschin wschin Apr 15, 2019 •
edited by sfilipi
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

public sealed class ColumnCopyingEstimator : TrivialEstimator<ColumnCopyingTransformer>
{
[BestFriend]
Expand Down Expand Up @@ -67,6 +82,9 @@ public override SchemaShape GetOutputSchema(SchemaShape inputSchema)
}
}

/// <summary>
/// <see cref="ITransformer"/> resulting from fitting an <see cref="ColumnCopyingEstimator"/>.
/// </summary>
public sealed class ColumnCopyingTransformer : OneToOneTransformerBase
{
[BestFriend]
Expand Down
20 changes: 13 additions & 7 deletions src/Microsoft.ML.Data/Transforms/ExtensionsCatalog.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,15 @@ internal static IReadOnlyList<InputOutputColumnPair> ConvertFromValueTuples((str
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"/>
Copy link
Contributor

@natke natke Apr 12, 2019 •
edited by sfilipi
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another nit: add "," before which #Resolved

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

@wschin wschin Apr 15, 2019 •
edited by sfilipi
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

@shmoradims shmoradims Apr 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/// 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?

Copy link
Contributor

@natke natke Apr 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// <param name="catalog">The transform's catalog.</param>
/// <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"/>.
/// This column's data type will be the same as that of the input column.</param>
/// <param name="inputColumnName">Name of the column to copy the data from.
/// This estimator operates over any data type.</param>
/// <example>
/// <format type="text/markdown">
/// <![CDATA[
Expand All @@ -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" />
Copy link
Contributor

@natke natke Apr 12, 2019 •
edited by sfilipi
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add "," before which #Resolved

/// to a new column: <see cref="InputOutputColumnPair.OutputColumnName" />.
/// </summary>
/// <param name="catalog">The transform's catalog</param>
/// <param name="columns">The pairs of input and output columns.</param>
/// <remarks>This transform can operate over several columns.
/// For more information see <see cref="ColumnCopyingEstimator"/>.</remarks>
/// <param name="catalog">The transform's catalog.</param>
/// <param name="columns">The pairs of input and output columns. This estimator operates over any data type.</param>
/// <example>
/// <format type="text/markdown">
/// <![CDATA[
Expand Down