Xml docs for trainers and a minor infrastructure changes #455

sfilipi · 2018-06-29T21:29:46Z

Addresses: #388

This PR adds an EntryPointInfo attribute that will contain the XML documentation to append to the summary: "Description", "References", "See also" sections.
It also modifies the C# generation code to append the content of this new attribute to the summary.

…ted Summary class XML. Adding documentation details and references for another batch of the trainers.

sfilipi · 2018-06-29T21:30:34Z

This is a first commit, to allow starting to review the changes.
I'll add another commit with more trainers documentation. #Resolved

…is no official documentation on using href with those tags.

TomFinley · 2018-07-02T16:12:49Z

src/Microsoft.ML.KMeansClustering/KMeansPlusPlusTrainer.cs

-        internal const string Summary = "K-means is a popular clustering algorithm. With K-means, the data is clustered into a specified "
-            + "number of clusters in order to minimize the within-cluster sum of squares. K-means++ improves upon K-means by using a better "
-            + "method for choosing the initial cluster centers.";
+        internal const string Summary = "KMeans++ clustering algorithm";


internal const string Summary = "KMeans++ clustering algorithm"; [](start = 8, length = 64)

This summary isn't quite as useful. I understand that we're adding more descriptive text on remarks for the API, but this is a temporary solution as noted elsewhere, so why are we degrading the text that we do have? #Resolved

The other summaries are just one liner, like: ''Averaged Perceptron Binary Classifier." or "Train a field-aware factorization machine for binary classification".
This felt like the outlier. Restoring it.

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

Well, I don't know. I think people don't write good summaries because they're lazy, I wouldn't want to normalize that laziness when someone actually did a good job writing a summary. :) #Resolved

TomFinley · 2018-07-02T16:14:45Z

src/Microsoft.ML.LightGBM/LightGbmBinaryTrainer.cs

@@ -11,7 +11,7 @@
 using Microsoft.ML.Runtime.LightGBM;
 using Microsoft.ML.Runtime.Model;

-[assembly: LoadableClass(LightGbmBinaryTrainer.Summary, typeof(LightGbmBinaryTrainer), typeof(LightGbmArguments),
+[assembly: LoadableClass(LightGbmBinaryTrainer.UserName, typeof(LightGbmBinaryTrainer), typeof(LightGbmArguments),


UserName [](start = 47, length = 8)

The first thing I believe is the summary, or should be. If it was badly written, that's fine, we ought to improve it, but to use the user-name as the first argument is wrong.

Also the thing that is actually being filled in as the user name is a string literal (that somehow differs from the field declared as UserName. #Resolved

TomFinley · 2018-07-02T16:16:31Z

src/Microsoft.ML.StandardLearners/Standard/LinearClassificationTrainer.cs

-Elastic net regularization can be specified by the l2_weight and l1_weight parameters. Note that the l2_weight has an effect on the rate of convergence. 
-In general, the larger the l2_weight, the faster SDCA converges.";
+The results depends on the order of the training data. For reproducible results, it is recommended that one sets <paramref>shuffle</paramref> to
+False and <paramref>NumThreads</paramref> to 1.


NumThreads [](start = 10, length = 31)

Is the correct form not <paramref href=?

Also this seems like kind of a strange solution... #Resolved

it's actually . Thanks for spotting it.

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

TomFinley · 2018-07-02T16:45:32Z

src/Microsoft.ML.FastTree/FastTree.cs

@@ -82,6 +82,31 @@ public abstract class FastTreeTrainerBase<TArgs, TPredictor> :

        protected string InnerArgs => CmdParser.GetSettings(Host, Args, new TArgs());

+        internal const string Remarks = @"<remarks>
+<para>FastTrees is an implementation of FastRank. FastRank is an efficient implementation of the <a href='https://arxiv.org/abs/1505.01866'>MART</a> gradient boosting algorithm. 


"FastTrees is" -> "FastTree is"... Also they will have no idea what FastRank is. Bringing in FastRank just complicates the picture a bit. The long history of Microsoft's internal tools, while possibly interesting to some people, does not belong in the first few sentences of user document. #Resolved

TomFinley · 2018-07-02T16:48:32Z

src/Microsoft.ML.StandardLearners/Standard/LinearClassificationTrainer.cs

-In general, the larger the l2_weight, the faster SDCA converges.";
+The results depends on the order of the training data. For reproducible results, it is recommended that one sets <paramref>shuffle</paramref> to
+False and <paramref>NumThreads</paramref> to 1.
+Elastic net regularization can be specified by the l2_weight and l1_weight parameters. Note that the <paramref>l2_weight</paramref> has an effect on the rate of convergence. 


Are l2_weight and l1_weight parameters? Those do not conform to .NET naming guidelines. Are they really named that? Also same note about <paramref href, maybe serach for <paramref> in your code to find and fix. #Resolved

yep, spotted the l2_weight and l1_weight and changed them to the actual L2Const and L1Threeshold.

The usage of paramref in the codebase is correct.

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

TomFinley · 2018-07-02T16:51:08Z

So, just curious @sfilipi , if I had this task of generating temporary XML comments for these classes, I wouldn't have done it this way. I would have written a separate file in the API itself, with the classes I wanted to document as partial classes (they already are partial to allow for these sorts of shenanigans), with comments directly on the partial classes. We already use the partial trick class for this sort of thing. That will have a few advantages:

Doesn't require this elaborate (and frankly strange) infrastructure,
When we get rid of the old API, we can just copy the comments over more or less directly, rather than reforming strings against as XML comments.

What do you think?

sfilipi · 2018-07-02T17:26:22Z

The documentation that i was aiming at improving was in the generated CSharpApi; the classes in Microsoft.ML.Trainers and Microsoft.ML.Transforms namespaces.
So the comments would still need to live somewhere, and the CSharp generator would need a pointer to them through the EntryPoints infrastructure.
That's why I kept them with the classes themselves and wired them to the generated C#Api, through the EntryPoints.

Writting another class with all the partial classes by hand just for the comments seems like prone to oversight when it comes to adding the documentation.

Reading more about the C# docs, there is an tag that we can leverage, we'd keep the XML docs in the docs side, in XML, and just use this tag to wire the path to those files to the CSharp Api.
@shauheen what is your opinion?
Example: https://docs.microsoft.com/en-us/dotnet/csharp/codedoc#ltincludegt

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

Fixing new line character in the ep_list and manifest.

TomFinley

Accepted provisionally, that we work on making this better at a later time.

shauheen · 2018-07-02T23:09:36Z

src/Microsoft.ML.StandardLearners/Standard/Online/AveragedPerceptron.cs

-        internal const string DetailedSummary = @"Perceptron is a classification algorithm that makes its predictions based on a linear function.
+        internal const string Summary = "Averaged Perceptron Binary Classifier.";
+        internal const string Remarks = @"<remarks>
+Perceptron is a classification algorithm that makes its predictions based on a linear function.
 I.e., for an instance with feature values f0, f1,..., f_D-1, , the prediction is given by the sign of sigma[0,D-1] ( w_i * f_i), where w_0, w_1,...,w_D-1 are the weights computed by the algorithm.


I.e., for an instance [](start = 0, length = 21)

This is very hard to read. No need to fix now though. #Closed

shauheen

* updating the C# api generator to append the Remarks XML to the generated Summary class XML. Adding documentation details and references for another batch of the trainers. * correcting test trigger condition * typo * More documentation * substituting the <see> and <seealso> tags with <a> tags, since there is no official documentation on using href with those tags. * adressing PR comments. Fixing new line character in the ep_list and manifest. * Fixing the list and code generation merges from master

sfilipi added 2 commits June 29, 2018 14:22

updating the C# api generator to append the Remarks XML to the genera…

3f5045c

…ted Summary class XML. Adding documentation details and references for another batch of the trainers.

correcting test trigger condition

06b1416

sfilipi requested review from CESARDELATORRE, GalOshri, shauheen, TomFinley and glebuk June 29, 2018 21:29

sfilipi added 3 commits June 29, 2018 14:31

typo

c18d254

More documentation

3b57e44

substituting the <see> and <seealso> tags with <a> tags, since there …

af18b54

…is no official documentation on using href with those tags.

TomFinley reviewed Jul 2, 2018

View reviewed changes

sfilipi added 3 commits July 2, 2018 11:23

adressing PR comments.

b1301d8

Fixing new line character in the ep_list and manifest.

merging from master

ad57632

Fixing the list and code generation merges from master

5dcce44

TomFinley approved these changes Jul 2, 2018

View reviewed changes

shauheen reviewed Jul 2, 2018

View reviewed changes

sfilipi mentioned this pull request Jul 2, 2018

Additional documentation for the ML.Net types and components should live in xml docs #477

Closed

shauheen approved these changes Jul 2, 2018

View reviewed changes

sfilipi merged commit b9086b6 into dotnet:master Jul 2, 2018

sfilipi deleted the xmlDocs branch July 2, 2018 23:15

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Xml docs for trainers and a minor infrastructure changes #455

Xml docs for trainers and a minor infrastructure changes #455

Uh oh!

sfilipi commented Jun 29, 2018

Uh oh!

sfilipi commented Jun 29, 2018 •

edited

Loading

Uh oh!

TomFinley Jul 2, 2018 •

edited by sfilipi

Loading

Uh oh!

sfilipi Jul 2, 2018

Uh oh!

TomFinley Jul 2, 2018 •

edited by sfilipi

Loading

Uh oh!

TomFinley Jul 2, 2018 •

edited by sfilipi

Loading

Uh oh!

TomFinley Jul 2, 2018 •

edited by sfilipi

Loading

Uh oh!

sfilipi Jul 2, 2018

Uh oh!

TomFinley Jul 2, 2018 •

edited by sfilipi

Loading

Uh oh!

TomFinley Jul 2, 2018 •

edited by sfilipi

Loading

Uh oh!

sfilipi Jul 2, 2018

Uh oh!

TomFinley commented Jul 2, 2018 •

edited

Loading

Uh oh!

sfilipi commented Jul 2, 2018 •

edited

Loading

Uh oh!

TomFinley left a comment

Uh oh!

shauheen Jul 2, 2018 •

edited

Loading

Uh oh!

shauheen left a comment

Uh oh!

Uh oh!

Xml docs for trainers and a minor infrastructure changes #455

Xml docs for trainers and a minor infrastructure changes #455

Uh oh!

Conversation

sfilipi commented Jun 29, 2018

Uh oh!

sfilipi commented Jun 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfilipi Jul 2, 2018

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfilipi Jul 2, 2018

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfilipi Jul 2, 2018

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

TomFinley left a comment

Choose a reason for hiding this comment

Uh oh!

shauheen Jul 2, 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!

shauheen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sfilipi commented Jun 29, 2018 •

edited

Loading

TomFinley Jul 2, 2018 •

edited by sfilipi

Loading

TomFinley Jul 2, 2018 •

edited by sfilipi

Loading

TomFinley Jul 2, 2018 •

edited by sfilipi

Loading

TomFinley Jul 2, 2018 •

edited by sfilipi

Loading

TomFinley Jul 2, 2018 •

edited by sfilipi

Loading

TomFinley Jul 2, 2018 •

edited by sfilipi

Loading

TomFinley commented Jul 2, 2018 •

edited

Loading

sfilipi commented Jul 2, 2018 •

edited

Loading

shauheen Jul 2, 2018 •

edited

Loading