-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Reformatting Conversion, FeatureSelection and Image Analytics of Transform to Width 85 #3943
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
Conversation
// Display original column 'Survived' (boolean) and converted column | ||
// SurvivedInt32' (Int32) | ||
var convertedData = mlContext.Data.CreateEnumerable<TransformedData>( | ||
transformedData, true); | ||
foreach (var item in convertedData) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new line between any two lines that were broken down into multiple lines but no new line between multiple breakdowns of a single line.
// full map. If that parameter is left to the default 0 value, the | ||
// mapping is not preserved. | ||
var pipeline = mlContext.Transforms.Conversion.Hash("CategoryHashed", | ||
"Category", numberOfBits: 16, maximumNumberOfInverts: -1).Append( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.Append( [](start = 73, length = 8)
I would move .Append to a new line. to be clear we are adding a new transform.
var defaultPipeline = mlContext.Transforms.Text.TokenizeIntoWords( | ||
"TokenizedText", nameof(DataPoint.Review)).Append(mlContext | ||
.Transforms.Conversion.MapValueToKey(nameof(TransformedData.Keys), | ||
"TokenizedText")); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please move .Append to a new line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requesting changes. The readability of these reformat pull requests is especially bad. I left a bunch of notes in one of the earlier ones, but those notes do not cover all situations where this is worse than before.
Thank you for all of your comments and suggestions. We are going to review and check-in these PRs to address the issue of the 85 character limit. We will open a new issue with subsequent improvements to implement the style guides you had suggested. |
Console.WriteLine($"{featureRow.StudyTime}\t\t{featureRow.StudyTimeCategory}\t\t\t{featureRow.Course}\t\t{featureRow.CourseCategory}"); | ||
Console.WriteLine($"{featureRow.StudyTime}\t\t" + | ||
$"{featureRow.StudyTimeCategory}\t\t\t{featureRow.Course}\t\t" + | ||
$"{featureRow.CourseCategory}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indent?
docs/samples/Microsoft.ML.Samples/Dynamic/Transforms/ImageAnalytics/ResizeImages.cs
Show resolved
Hide resolved
…sform to Width 85 (dotnet#3943) * samples/dynamic/transforms/conversions formatted to 85 char * samples/dynamic/transforms/featureselection formatted to 85 char * ImageAnalytics * ImageAnalytics * small corrections to formatting * minor tab and empty line changes
Guidelines followed:
-85 characters per line
-Use 4 spaces for indentation
-Dot and open parentheses stay on same line as function
-If not a preexisting line under line that we break, add an extra line after it
-Don't indent comments
-Don't break a comment if it represents output
-Don't break links
-If applicable, break right before $
-Keep math op together
Fix for issue #3478