Skip to content
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

Jump to bottom

Reformatted Regression samples to width 85 #3948

Merged
merged 4 commits into from
Jul 3, 2019
Merged

Reformatted Regression samples to width 85 #3948

merged 4 commits into from
Jul 3, 2019

Conversation

sayanshaw24
Copy link
Contributor

Guidelines followed:

  • 85 characters per line
  • Use 4 spaces for indentation
  • Dot, open parentheses, and function/variable name on same line
  • Math operations on same line
  • Don't indent comments
  • Don't break links
  • Don't break a comment if it represents a print output
  • Add an extra line after a break if it does not already exist
  • Break before "$"
    Fix for Issue Samples width: they need to be formatted at 85 characters or less. #3478

codemzs
codemzs reviewed Jul 2, 2019
View reviewed changes
docs/samples/Microsoft.ML.Samples/Dynamic/Trainers/Regression/FastForest.cs Outdated
// Create testing data. Use different random seed to make it different from training data.
var testData = mlContext.Data.LoadFromEnumerable(GenerateRandomDataPoints(5, seed: 123));
// Create testing data. Use different random seed to make it different
// from training data.
Copy link
Member

Choose a reason for hiding this comment

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

  	 [](start = 0, length = 3)

please use four spaces instead of tabs. https://blogs.msdn.microsoft.com/zainnab/2010/03/14/how-to-convert-tabs-to-spaces-and-vice-versa/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sorry, I think I missed a few lines while untabifying the tt files. Will fix that and commit.

codemzs
codemzs approved these changes Jul 2, 2019
View reviewed changes
Copy link
Member

@codemzs codemzs left a comment

Choose a reason for hiding this comment

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

:shipit:

rayankrish
rayankrish reviewed Jul 2, 2019
View reviewed changes
docs/samples/Microsoft.ML.Samples/Dynamic/Trainers/Regression/FastForest.cs Outdated
@@ -8,37 +8,47 @@ namespace Samples.Dynamic.Trainers.Regression
{
public static class FastForestRegression
{
// This example requires installation of additional NuGet package
// <a href="https://www.nuget.org/packages/Microsoft.ML.FastTree/">Microsoft.ML.FastTree</a>.

Copy link

Choose a reason for hiding this comment

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

There's an extra line here that I don't think you need.

sierralee51
sierralee51 reviewed Jul 2, 2019
View reviewed changes
docs/samples/Microsoft.ML.Samples/Dynamic/Trainers/Regression/FastForestWithOptions.cs Outdated
@@ -9,19 +9,23 @@ namespace Samples.Dynamic.Trainers.Regression
{
public static class FastForestWithOptionsRegression
{
// This example requires installation of additional NuGet package
// <a href="https://www.nuget.org/packages/Microsoft.ML.FastTree/">Microsoft.ML.FastTree</a>.

Copy link
Contributor

Choose a reason for hiding this comment

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

extra line here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep @RadicalRayan explained why below. I think he's asking Zeeshan about the feasibility of removing the line from all of the tt files with the NuGet package installation comment.

sierralee51
sierralee51 reviewed Jul 2, 2019
View reviewed changes
docs/samples/Microsoft.ML.Samples/Dynamic/Trainers/Regression/FastForestWithOptions.cs Outdated
// Create testing data. Use different random seed to make it different from training data.
var testData = mlContext.Data.LoadFromEnumerable(GenerateRandomDataPoints(5, seed: 123));
// Create testing data. Use different random seed to make it different
// from training data.
Copy link
Contributor

Choose a reason for hiding this comment

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

tab issue here

rayankrish
rayankrish reviewed Jul 2, 2019
View reviewed changes
docs/samples/Microsoft.ML.Samples/Dynamic/Trainers/Regression/FastForest.tt Outdated
string ClassHeader = @"// This example requires installation of additional NuGet package
// <a href=""https://www.nuget.org/packages/Microsoft.ML.FastTree/"">Microsoft.ML.FastTree</a>. ";
string ClassHeader = @"
// This example requires installation of additional NuGet package for
Copy link

Choose a reason for hiding this comment

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

Since you put this on a new line, it added an extra empty line for all the .cs files that use this .tt file. If you fix this and run custom tool, it'll fix the other cases too (I commented about one)

sierralee51
sierralee51 reviewed Jul 2, 2019
View reviewed changes
docs/samples/Microsoft.ML.Samples/Dynamic/Trainers/Regression/FastTreeTweedie.cs Outdated
// Create testing data. Use different random seed to make it different from training data.
var testData = mlContext.Data.LoadFromEnumerable(GenerateRandomDataPoints(5, seed: 123));
// Create testing data. Use different random seed to make it different
// from training data.
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this tabbing issue may be linked to a .tt or .ttinclude file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah they're all fixed in the new commit.

rayankrish
rayankrish reviewed Jul 2, 2019
View reviewed changes
docs/samples/Microsoft.ML.Samples/Dynamic/Trainers/Regression/FastForestWithOptions.tt Outdated
string ClassHeader = @"// This example requires installation of additional NuGet package
// <a href=""https://www.nuget.org/packages/Microsoft.ML.FastTree/"">Microsoft.ML.FastTree</a>. ";
string ClassHeader = @"
// This example requires installation of additional NuGet package for
Copy link

Choose a reason for hiding this comment

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

I think this will cause the same problem too.

rayankrish
rayankrish reviewed Jul 2, 2019
View reviewed changes
docs/samples/Microsoft.ML.Samples/Dynamic/Trainers/Regression/FastTreeTweedieWithOptions.tt Outdated
@@ -12,7 +14,9 @@ string TrainerOptions = @"FastTreeTweedieTrainer.Options
LabelColumnName = nameof(DataPoint.Label),
FeatureColumnName = nameof(DataPoint.Features),
// Use L2Norm for early stopping.
EarlyStoppingMetric = Microsoft.ML.Trainers.FastTree.EarlyStoppingMetric.L2Norm,
EarlyStoppingMetric =
Microsoft.ML.Trainers.FastTree.EarlyStoppingMetric.L2Norm,
Copy link

Choose a reason for hiding this comment

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

tabbing issue

rayankrish
rayankrish reviewed Jul 2, 2019
View reviewed changes
docs/samples/Microsoft.ML.Samples/Dynamic/Trainers/Regression/FastTreeWithOptions.tt Outdated
// Use L2-norm for early stopping. If the gradient's L2-norm is
// smaller than an auto-computed value, training process will stop.
EarlyStoppingMetric =
Microsoft.ML.Trainers.FastTree.EarlyStoppingMetric.L2Norm,
Copy link

Choose a reason for hiding this comment

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

this is the root of the tab issue seen in other .cs files too

sierralee51
sierralee51 reviewed Jul 2, 2019
View reviewed changes
docs/samples/Microsoft.ML.Samples/Dynamic/Trainers/Regression/Gam.tt
string Trainer = @"Gam(labelColumnName: nameof(DataPoint.Label), featureColumnName: nameof(DataPoint.Features))";
string Trainer = @"Gam(
labelColumnName: nameof(DataPoint.Label),
featureColumnName: nameof(DataPoint.Features))";
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct? It seems like there are a lot of spaces before it

sierralee51
sierralee51 reviewed Jul 2, 2019
View reviewed changes
docs/samples/Microsoft.ML.Samples/Dynamic/Trainers/Regression/LightGbmAdvanced.cs
labelColumnName: labelName,
numberOfLeaves: 4,
minimumExampleCountPerLeaf: 6,
learningRate: 0.001));
Copy link
Contributor

Choose a reason for hiding this comment

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

there are a lot of spaces in front of this

Copy link

Choose a reason for hiding this comment

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

            var pipeline = mlContext.Transforms.Concatenate("Features",
                   featureNames)
                   .Append(mlContext.Regression.Trainers.LightGbm(
                       labelColumnName: labelName,
                       numberOfLeaves: 4,
                       minimumExampleCountPerLeaf: 6,
                       learningRate: 0.001));

sierralee51
sierralee51 reviewed Jul 2, 2019
View reviewed changes
docs/samples/Microsoft.ML.Samples/Dynamic/Trainers/Regression/OnlineGradientDescent.cs Outdated
foreach (var p in predictions)
Console.WriteLine($"Label: {p.Label:F3}, Prediction: {p.Score:F3}");

// This trainer is not numerically stable. Please see issue #2425.

// This trainer is not numerically stable. Please see issue #2425.
Copy link
Contributor

Choose a reason for hiding this comment

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

extra line before. Needs another four spaces to align with the other comments

rayankrish
rayankrish reviewed Jul 2, 2019
View reviewed changes
docs/samples/Microsoft.ML.Samples/Dynamic/Trainers/Regression/LightGbmAdvanced.cs
var metrics = mlContext.Regression.Evaluate(dataWithPredictions, labelColumnName: labelName);
var metrics = mlContext.Regression.Evaluate(
dataWithPredictions,
labelColumnName: labelName);
Copy link

Choose a reason for hiding this comment

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

needs an empty line after

sierralee51
sierralee51 reviewed Jul 2, 2019
View reviewed changes
docs/samples/Microsoft.ML.Samples/Dynamic/Trainers/Regression/LightGbmAdvanced.cs
labelColumnName: labelName,
numberOfLeaves: 4,
minimumExampleCountPerLeaf: 6,
learningRate: 0.001));
Copy link
Contributor

Choose a reason for hiding this comment

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

again a lot of spaces

rayankrish
rayankrish reviewed Jul 2, 2019
View reviewed changes
...samples/Microsoft.ML.Samples/Dynamic/Trainers/Regression/OnlineGradientDescentWithOptions.cs Outdated
foreach (var p in predictions)
Console.WriteLine($"Label: {p.Label:F3}, Prediction: {p.Score:F3}");

// This trainer is not numerically stable. Please see issue #2425.

// This trainer is not numerically stable. Please see issue #2425.
Copy link

Choose a reason for hiding this comment

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

no need for empty line on 66. Needs spaces to align with the other comments.

rayankrish
rayankrish reviewed Jul 2, 2019
View reviewed changes
...samples/Microsoft.ML.Samples/Dynamic/Trainers/Regression/OnlineGradientDescentWithOptions.cs Outdated

// Evaluate the overall metrics
var metrics = mlContext.Regression.Evaluate(transformedTestData);
PrintMetrics(metrics);

// This trainer is not numerically stable. Please see issue #2425.

// This trainer is not numerically stable. Please see issue #2425.
Copy link

Choose a reason for hiding this comment

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

no need for empty line on 73. needs spaces to align with the other comments

rayankrish
rayankrish reviewed Jul 2, 2019
View reviewed changes
docs/samples/Microsoft.ML.Samples/Dynamic/Trainers/Regression/PermutationFeatureImportance.cs

.OrderByDescending(feature => Math.Abs(
feature.RootMeanSquaredError.Mean))

Copy link

Choose a reason for hiding this comment

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

lines 54 and 57 are empty lines. the tabbing looks good but you don't need the extra lines.

@sayanshaw24 sayanshaw24 merged commit 2da72cb into dotnet:master Jul 3, 2019
Dmitry-A pushed a commit to Dmitry-A/machinelearning that referenced this pull request Jul 24, 2019
@sayanshaw24 @Dmitry-A
Reformatted Regression samples to width 85 (dotnet#3948)
2db8465 
* Reformatted Regression samples

* Untabified comments in tt files.

* Removed extra lines

* Removed extra lines and added necessary indents
This was referenced Dec 13, 2019
Closed
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Mar 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sierralee51 sierralee51 sierralee51 left review comments

@rayankrish rayankrish rayankrish approved these changes

@codemzs codemzs codemzs approved these changes

@natke natke Awaiting requested review from natke

@wschin wschin Awaiting requested review from wschin

@artidoro artidoro Awaiting requested review from artidoro

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sayanshaw24 @codemzs @rayankrish @sierralee51