Comments added to LearningPipeline class to make Intellisense more helpful. #50

zeahmed · 2018-05-07T19:12:55Z

Fixing issue #31 in this PR.

Update my fork

…36: Intellisense is not helpful with filling in pipeline components.

eerhardt · 2018-05-07T21:16:21Z

src/Microsoft.ML/LearningPipeline.cs

+    /// <summary>
+    /// LearningPipeline class is used to define the steps needed to perform desired machine learning task.<para/>
+    /// The steps are defined by adding a data loader (e.g. <see cref="TextLoader"/>) followed by zero or more transforms (e.g. <see cref="Microsoft.ML.Transforms.TextFeaturizer"/>) 
+    /// and atmost one trainer/learner (e.g. <see cref="Microsoft.ML.Trainers.FastTreeBinaryClassifier"/>) in the pipeline.


type-o atmost should be 2 words.

eerhardt · 2018-05-07T21:23:34Z

src/Microsoft.ML/LearningPipeline.cs

+    /// The steps are defined by adding a data loader (e.g. <see cref="TextLoader"/>) followed by zero or more transforms (e.g. <see cref="Microsoft.ML.Transforms.TextFeaturizer"/>) 
+    /// and atmost one trainer/learner (e.g. <see cref="Microsoft.ML.Trainers.FastTreeBinaryClassifier"/>) in the pipeline.
+    /// 
+    /// Data can be analyzed at every step by inspecting the LearningPipeline object in VS.Net debugger.


I'm not sure this line provides much value. Do you think we need it?

eerhardt · 2018-05-07T21:25:54Z

src/Microsoft.ML/LearningPipeline.cs

+    /// and atmost one trainer/learner (e.g. <see cref="Microsoft.ML.Trainers.FastTreeBinaryClassifier"/>) in the pipeline.
+    /// 
+    /// Data can be analyzed at every step by inspecting the LearningPipeline object in VS.Net debugger.
+    /// <example>


In https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/xmldoc/example it shows that <example> is not nested inside the <summary> section. Instead, it is its own top-level section. #Resolved

eerhardt · 2018-05-07T21:28:39Z

src/Microsoft.ML/LearningPipeline.cs

+        /// </summary>
+        /// <typeparam name="TInput">Type of data instances the model will be trained on. It's a custom type defined by the user according to the structure of data.
+        /// <para/>
+        /// E.g. please see "Microsoft.ML.Scenarios.ScenarioTests.SentimentData" in "Microsoft.ML.Tests.csproj" for input type definition for sentiment classification task.


I don't think typical users will have access to the repo. So putting a reference like this in the XML ref docs wouldn't help that much. Maybe a FWLink to the 'getting started' doc instead?

TomFinley · 2018-05-07T22:26:25Z

src/Microsoft.ML/LearningPipeline.cs

+        /// <summary>
+        /// Check if a specific loader/transform/trainer is in the pipeline?
+        /// </summary>
+        /// <param name="item">Any ML component (data loader, transform or trainer) defined as ILearningPipelineItem.</param>


ILearningPipelineItem [](start = 95, length = 21)

When having type names in documentation, it may be good practice for us to use the <see tags, so this would become <see cref="ILearningPipelineItem" />. The reason is: the XML docs will be generated with links, and attempts to, say, rename the type in VS will track these usages as well. #Closed

TomFinley · 2018-05-07T22:30:04Z

src/Microsoft.ML/LearningPipeline.cs

+        /// Copy the pipeline items into an array.
+        /// </summary>
+        /// <param name="array">Array the items are copied to.</param>
+        /// <param name="arrayIndex">Index to start copying from.</param>


Index to start copying from [](start = 37, length = 27)

The phrase "Index to start copying from", the "from" suggests that this is an index on the source, but rather, it is an index on the destination.

Perhaps the word "into" vs. "from" would be more clear... plus an explicit paramref, just to make things absolutely crystal clear. So something like maybe, Index of <paramref name="array" /> we start copying to. I imagine it could be wordsmithed better. :) #Closed

I typically copy the MSDN docs for comments like this. See ICollection.CopyTo

array
Type: System.Array
The one-dimensional Array that is the destination of the elements copied from ICollection. The Array must have zero-based indexing.

index
Type: System.Int32
The zero-based index in array at which copying begins.

TomFinley · 2018-05-07T22:31:32Z

src/Microsoft.ML/LearningPipeline.cs


+
+    /// <summary>
+    /// LearningPipeline class is used to define the steps needed to perform desired machine learning task.<para/>


perform desired [](start = 69, length = 15)

"perform desired" => "perform a desired"? #Closed

TomFinley · 2018-05-07T22:34:04Z

src/Microsoft.ML/LearningPipeline.cs


+
+    /// <summary>
+    /// LearningPipeline class is used to define the steps needed to perform desired machine learning task.<para/>


LearningPipeline [](start = 8, length = 16)

Class names should appear in <see tags`. However in this case, given that this is describing this class, I might just replace "LearningPipeline class is used" with "This class is used..."

TomFinley · 2018-05-07T22:35:40Z

src/Microsoft.ML/LearningPipeline.cs

        private List<ILearningPipelineItem> Items { get; } = new List<ILearningPipelineItem>();

+        /// <summary>
+        /// Construct an empty LearningPipeline object.


LearningPipeline [](start = 31, length = 16)

Generally use <see here. #Closed

TomFinley · 2018-05-07T22:36:58Z

Thank you very much for writing code on this important class @zeahmed ! #Closed

TomFinley · 2018-05-07T22:52:52Z

src/Microsoft.ML/LearningPipeline.cs

+        /// Check if a specific loader/transform/trainer is in the pipeline?
+        /// </summary>
+        /// <param name="item">Any ML component (data loader, transform or trainer) defined as ILearningPipelineItem.</param>
+        /// <returns>true/false</returns>


returns>true/false</returns [](start = 13, length = 27)

That the return value is either true or false is implied by the return value being bool. Can this be improved somehow? It seems like any "improvement" would just involve restating what what already written in the description, so is it acceptable in situations like this to just not include a <returns> tag? #Closed

Again, I typically copy the MSDN docs: https://msdn.microsoft.com/en-us/library/k5cf1d56(v=vs.110).aspx

Return Value
Type: System.Boolean
true if item is found in the ICollection; otherwise, false.

TomFinley · 2018-05-07T22:53:28Z

src/Microsoft.ML/LearningPipeline.cs

        public void Add(ILearningPipelineItem item) => Items.Add(item);
+
+        /// <summary>
+        /// Remove all the transforms/trainers from the pipeline.


transforms/trainers [](start = 27, length = 19)

Not loaders? It seems like this would also clear any loaders. #Closed

zeahmed · 2018-05-08T00:51:41Z

Thanks for the feedback! I updated the PR.

eerhardt

Looks good. Thanks, @zeahmed!

TomFinley · 2018-05-08T20:01:43Z

src/Microsoft.ML/LearningPipeline.cs


+
+    /// <summary>
+    /// <see cref="LearningPipeline"/> class is used to define the steps needed to perform a desired machine learning task.<para/>


class [](start = 8, length = 36)

I'd really prefer "This class", but if you really want the actual name of the class referenced, for grammar reasons maybe prefix with "The" so that it's The LearningPipeline class" vs. "LearningPipeline class."

TomFinley

zeahmed and others added 7 commits May 4, 2018 14:50

Merge pull request #1 from dotnet/master

d80ade9

Update my fork

Comments added to LearningPipeline class in accordance with #Bug 2406…

7856abd

…36: Intellisense is not helpful with filling in pipeline components.

Merge branch 'master' into comments_learningpipeline

d090170

Comments added to LearningPipeline class in accordance with #Bug 2406…

77d21ff

…36: Intellisense is not helpful with filling in pipeline components.

Fixed a typo in namespace

5471ed1

Merge branch 'master' of https://github.com/zeahmed/machinelearning

203d9fe

Merge branch 'master' into comments_learningpipeline

5c67672

zeahmed requested review from codemzs, GalOshri, shauheen and eerhardt May 7, 2018 19:41

eerhardt reviewed May 7, 2018

View reviewed changes

TomFinley reviewed May 7, 2018

View reviewed changes

Addressed reviewers' comments.

98fc8d0

Addressed reviewers' comments.

e7dc707

eerhardt approved these changes May 8, 2018

View reviewed changes

TomFinley reviewed May 8, 2018

View reviewed changes

TomFinley approved these changes May 8, 2018

View reviewed changes

Addressed reviewers' comments.

5089e98

eerhardt merged commit 3e5b5ed into dotnet:master May 9, 2018

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



		/// <summary>
		/// LearningPipeline class is used to define the steps needed to perform desired machine learning task.<para/>



		/// <summary>
		/// <see cref="LearningPipeline"/> class is used to define the steps needed to perform a desired machine learning task.<para/>

Comments added to LearningPipeline class to make Intellisense more helpful. #50

Comments added to LearningPipeline class to make Intellisense more helpful. #50

Uh oh!

Conversation

zeahmed commented May 7, 2018

Uh oh!

eerhardt May 7, 2018

Choose a reason for hiding this comment

Uh oh!

eerhardt May 7, 2018

Choose a reason for hiding this comment

Uh oh!

eerhardt May 7, 2018 • edited by glebuk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt May 7, 2018

Choose a reason for hiding this comment

Uh oh!

TomFinley May 7, 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 May 7, 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!

eerhardt May 8, 2018

Choose a reason for hiding this comment

Uh oh!

TomFinley May 7, 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 May 7, 2018

Choose a reason for hiding this comment

Uh oh!

TomFinley May 7, 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 commented May 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TomFinley May 7, 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!

eerhardt May 8, 2018

Choose a reason for hiding this comment

Uh oh!

TomFinley May 7, 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 May 8, 2018

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

TomFinley May 8, 2018

Choose a reason for hiding this comment

Uh oh!

TomFinley left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eerhardt May 7, 2018 •

edited by glebuk

Loading

TomFinley May 7, 2018 •

edited

Loading

TomFinley May 7, 2018 •

edited

Loading

TomFinley May 7, 2018 •

edited

Loading

TomFinley May 7, 2018 •

edited

Loading

TomFinley commented May 7, 2018 •

edited

Loading

TomFinley May 7, 2018 •

edited

Loading

TomFinley May 7, 2018 •

edited

Loading