Skip to content

XML strings for the documentation should live outside of the src code, in xml files. #510

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.

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 9 commits into from
Jul 11, 2018

Conversation

sfilipi
Copy link
Member

@sfilipi sfilipi commented Jul 9, 2018

Resolves #477 by moving the strings with the XML remarks, examples etc into a separate XML file.
Now the actual classes and their C# Api counterparts share the path to the XML node that contains the documentation and the respective example.

On this PR, there are only two examples that are separate from the rest of the descriptive xml: the LogisticRegressionBinaryClassifier and LogisticRegressionClassifier, to give an idea of the infrastructure.

The rest of the examples are coming in the next PR, together with the rest of the documentation.

sfilipi added 2 commits July 9, 2018 10:39
…les.

For the summary text that is common between several learners, the examples will be added on a separate node.
An example of how that will look like is in the LogisticRegressionBinaryClassifier and LogisticRegressionClassifier.
@TomFinley
Copy link
Contributor

TomFinley commented Jul 9, 2018

Thanks for doing this @sfilipi ! So what are best practices here? I might have expected that any XML include files would be in the same directory or some subdirectory as the project with the relevant code, as opposed to some other location way off in the hinterlands not even in the "source" files.

Secondary reason for my preference is that it might be nice if docs remain something human browsable and comprehensible.. right now it is that since they're just markdown files (which are human readable). This stuff, despite being "documentation" of a sort, should probably be closer to the source. #Resolved

@TomFinley
Copy link
Contributor

That is, I'd expect the FastTree XML docs to be somewhere in the FastTree project, LightGBM in lightGBM project, etc... rather than having master files where all documentation lives. I'm not sure I consider these massive files maintainable.


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

@sfilipi
Copy link
Member Author

sfilipi commented Jul 9, 2018

Was on the fence between single file, per component type, and per-project files.
I started with per-project, hence the PCA file. I'll update and break down, since the votes so far are 1.5 - 0.5 in favor of per-project files.


In reply to: 403605251 [](ancestors = 403605251,403604163)

@TomFinley
Copy link
Contributor

Hmmmm. :) Sorry what votes?


In reply to: 403606882 [](ancestors = 403606882,403605251,403604163)

@sfilipi
Copy link
Member Author

sfilipi commented Jul 9, 2018

The OSX failures are unrelated to my changes. They fail to retrieve some of the nuget dependancies. #Resolved

@@ -20,6 +20,7 @@ public interface IFastTreeTrainerFactory : IComponentFactory<ITrainer>
{
}

/// <include file='./XMLDoc.xml' path='docs/members/member[@name="FastTree"]/*' />
Copy link
Contributor

@TomFinley TomFinley Jul 10, 2018

Choose a reason for hiding this comment

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

XMLDoc [](start = 25, length = 6)

I might be being a little OCD, but the redundancy of the name XMLDoc.xml just drives me absolutely bonkers. Maybe just plain old doc.xml (lower case doc)? #Resolved

</summary>
<remarks>
<para>
FastTrees is an efficient implementation of the <a href='https://arxiv.org/abs/1505.01866'>MART</a> gradient boosting algorithm.
Copy link
Contributor

@TomFinley TomFinley Jul 10, 2018

Choose a reason for hiding this comment

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

FastTrees [](start = 10, length = 9)

FastTree, not FastTrees. #Resolved

@@ -529,9 +529,9 @@ public sealed class EntryPointAttribute : Attribute
public string ShortName { get; set; }

/// <summary>
/// Remarks on the Entry Point, for more extensive XML documentation on the C#API
/// The path to the XML documentation on the C#API component
Copy link
Contributor

Choose a reason for hiding this comment

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

C#API [](start = 57, length = 5)

C# API?

</summary>
<remarks>
<para>
FastTree is an efficient implementation of the <a href='https://arxiv.org/abs/1505.01866'>MART</a> gradient boosting algorithm.
Copy link
Contributor

Choose a reason for hiding this comment

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

MART [](start = 100, length = 4)

This is a bit confusing, We say MART but the paper is an improved version DART. @tfinley@gmail.com do you know what is the correct one?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we use the acronym "DART" I think we need to have a link to that paper, and also explain what is meant by that. I think it's fine to just say MART honestly. The DART capabilities could perhaps be mentioned.

Copy link
Contributor

@shauheen shauheen left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Contributor

@TomFinley TomFinley left a comment

Choose a reason for hiding this comment

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

Thanks @sfilipi !

@sfilipi sfilipi merged commit d3004e6 into dotnet:master Jul 11, 2018
@sfilipi sfilipi deleted the transformsDocs branch July 11, 2018 23:20
ChiragRupani pushed a commit to ChiragRupani/machinelearning that referenced this pull request Jul 12, 2018
…, in xml files. (dotnet#510)

* Moving from xml strings to having the documentation details in xml files.
For the summary text that is common between several learners, the examples will be added on a separate node.
An example of how that will look like is in the LogisticRegressionBinaryClassifier and LogisticRegressionClassifier.

* fixing the aftermath of renaming the XML file.

* removing the Desc from the EntryPoint attribute is a bad idea.

* removing the XML docs from the doc folder, and added them under the respective projects.

* Some OS get picky about casing.

* file name should be vanilla

* Fixing comment
eerhardt pushed a commit to eerhardt/machinelearning that referenced this pull request Jul 27, 2018
…, in xml files. (dotnet#510)

* Moving from xml strings to having the documentation details in xml files.
For the summary text that is common between several learners, the examples will be added on a separate node.
An example of how that will look like is in the LogisticRegressionBinaryClassifier and LogisticRegressionClassifier.

* fixing the aftermath of renaming the XML file.

* removing the Desc from the EntryPoint attribute is a bad idea.

* removing the XML docs from the doc folder, and added them under the respective projects.

* Some OS get picky about casing.

* file name should be vanilla

* Fixing comment
@ghost ghost locked as resolved and limited conversation to collaborators Mar 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants