Reformatting ModelOperations and DataOperations samples to width 85 - #3923

Merged
sierralee51 merged 28 commits into
dotnet:masterfrom
sierralee51:samples
Jun 28, 2019
Merged

Reformatting ModelOperations and DataOperations samples to width 85#3923
sierralee51 merged 28 commits into
dotnet:masterfrom
sierralee51:samples

Conversation

@sierralee51

@sierralee51sierralee51 commented Jun 27, 2019•
edited
Loading

Copy link
Copy Markdown
Contributor

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

@dnfclas

dnfclas commented Jun 27, 2019•
edited
Loading

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

codemzs
codemzs reviewed Jun 27, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/ModelOperations/SaveLoadModel.cs Outdated

// Create a prediction engine from the model for feeding new data.
var engine = mlContext.Model.CreatePredictionEngine<Data, Transformation>(model);
var engine = mlContext.Model.CreatePredictionEngine<Data,

@codemzscodemzsJun 27, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@eerhardt Is it ok to break generics like this or should all of it be in the same line as its corresponding class?

@codemzscodemzsJun 27, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

CC: @natke

@sierralee51sierralee51Jun 27, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I'll fix it so that I split before the "<" in a later pull request. Thanks!

@codemzs
codemzs requested review from codemzs and eerhardtJune 27, 2019 17:23
@sierralee51
sierralee51 requested a review from natkeJune 27, 2019 17:32
@eerhardt

eerhardt commented Jun 27, 2019

Copy link
Copy Markdown
Member

85 characters per line

I kind of think that is too small. In dotnet/spark we are using 100. What's the reasoning for 85?

eerhardt
eerhardt reviewed Jun 27, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/ModelOperations/SaveLoadModelFile.cs Outdated
// Transform.
ITransformer model = mlContext.Transforms.Conversion.MapValueToKey(outputColumnName, inputColumnName).Fit(dataView);
ITransformer model = mlContext.Transforms.Conversion.MapValueToKey(
outputColumnName, inputColumnName).Fit(dataView);

@eerhardteerhardtJun 27, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For cases like this, I would rather we break per method call, than to break between a method and its parameter list.

For example:

// If you have chained method calls, line-break each method callEnumerable.Range(0,numRows).Select(i =>i.ToString()).ToArray();

That is much more readable than:

objecto=foo.methodCall(methodCallArg1,methodCallArg2).OtherMethodCall(arg3)

eerhardt
eerhardt reviewed Jun 27, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Program.cs Outdated

internal static void RunAll()
{
// Samples counter.

@eerhardteerhardtJun 27, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the usefulness of this comment? If those words make a better name, it would be better to rename the variable with those words, than to add a comment like this.

@sierralee51sierralee51Jun 27, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

My bad, I meant to delete that. That was for an earlier test.

@sierralee51

sierralee51 commented Jun 27, 2019

Copy link
Copy Markdown
ContributorAuthor

85 characters per line

I kind of think that is too small. In dotnet/spark we are using 100. What's the reasoning for 85?

@natke asked for 85 characters to ensure that a horizontal scrollbar is not needed on the docs webpage.

@eerhardt

eerhardt commented Jun 27, 2019

Copy link
Copy Markdown
Member

Ah, ok then I understand why 85 characters is better, it needs to be hosted inside a page with other things around it. Thanks for the explanation.

@sierralee51sierralee51 changed the title Reformatting ModelOperations samples to width 85Reformatting ModelOperations and DataOperations samples to width 85Jun 27, 2019
@sierralee51

sierralee51 commented Jun 28, 2019

Copy link
Copy Markdown
ContributorAuthor

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

*Updated to include DataOperations samples as well

justinormont
justinormont reviewed Jun 28, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/Cache.cs Outdated
sierralee51and others added 2 commits June 28, 2019 05:42
@sierralee51@justinormont
Update docs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/Cache.cs
10f40fd
Co-Authored-By: Justin Ormont <justinormont@users.noreply.github.com>
@sierralee51
Update Program.cs
00acf2a
Got rid of test comment
@codemzs
codemzs requested review from eerhardt and wschinJune 28, 2019 17:14
natke
natke reviewed Jun 28, 2019
View reviewed changes

@natkenatke left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Width of lines looks good.

Looks like there are quite a few places with extra whitespace at the beginning of the line

Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/DataViewEnumerable.cs Outdated
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/DataViewEnumerable.cs
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/DataViewEnumerable.tt Outdated
var rowEnumerableIgnoreMissing = mlContext.Data.CreateEnumerable<SampleTemperatureDataWithLatitude>(data,
reuseRowObject: true, ignoreMissingColumns: true);
// We can now examine the records in the IDataView. We first create an
// enumerable of rows in the IDataView.

@natkenatkeJun 28, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Extra whitespace

@sierralee51sierralee51Jun 28, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Should be fixed. Some tt files have certain indentations that insure that the comments in the cs files align.

@sierralee51
Update DataViewEnumerable.tt
acdf665
Fixed extra whitespace
@sierralee51
Update DataViewEnumerable.cs
842765f
Fixed extra whitespace
@sierralee51
Update DataViewEnumerable.tt
fe9c188
Fixed extra whitespace
@sierralee51
Update FilterRowsByColumn.tt
2f91a6d
Fixed whitespace
natke
natke approved these changes Jun 28, 2019
View reviewed changes

@natkenatke left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good! A couple of places where there is an extra carriage return

Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/FilterRowsByColumn.cs Outdated
{
Console.WriteLine($"{row.Date.ToString("d")}\t{row.Temperature}");
Console.WriteLine(
$"{row.Date.ToString("d")}\t{row.Temperature}");

@natkenatkeJun 28, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Extra carriage return here?

Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/FilterRowsByColumn.cs
foreach (var row in enumerable)
{
Console.WriteLine($"{row.Date.ToString("d")}\t{row.Temperature}");
Console.WriteLine(

@natkenatkeJun 28, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Extra carriage return?

Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/FilterRowsByColumn.tt Outdated
foreach (var row in enumerableOfData)
{
Console.WriteLine($""{row.Date.ToString(""d"")}\t{row.Temperature}"");
Console.WriteLine(

@natkenatkeJun 28, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Extra carriage return?

Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/FilterRowsByColumn.tt
foreach (var row in enumerable)
{
Console.WriteLine($""{row.Date.ToString(""d"")}\t{row.Temperature}"");
Console.WriteLine(

@natkenatkeJun 28, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Extra carriage return?

@sierralee51
sierralee51 merged commit 17c9155 into dotnet:masterJun 28, 2019
Dmitry-A pushed a commit to Dmitry-A/machinelearning that referenced this pull request Jul 24, 2019
@sierralee51@Dmitry-A
Reformatting ModelOperations and DataOperations samples to width 85 (d…
b31835d
…otnet#3923)
* Added a comment.
* reformatted ModelOperations samples to width 85
* Fixed commented-on parts of MachineOperations & reformatted DataOperations
* Update docs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/Cache.cs
Co-Authored-By: Justin Ormont <justinormont@users.noreply.github.com>
* Update Program.cs
Got rid of test comment
* Update DataViewEnumerable.tt
Fixed extra whitespace
* Update DataViewEnumerable.cs
Fixed extra whitespace
* Update DataViewEnumerable.tt
Fixed extra whitespace
* Update FilterRowsByColumn.tt
Fixed whitespace
* Update ShuffleRows.tt
Fixed whitespace
* Update TakeRows.tt
Fixed whitespace
* Update TakeRows.cs
Fixed whitespace
* Update SkipRows.cs
Fixed whitespace
* Update SkipRows.tt
Fixed whitespace
* Update ShuffleRows.cs
Fixed whitespace
* Update ShuffleRows.cs
Fixed whitespace
* Update ShuffleRows.cs
* Update ShuffleRows.tt
* Update SkipRows.tt
* Update SkipRows.cs
* Update FilterRowsByColumn.cs
Fixed whitespace
* Update FilterRowsByColumn.cs
Fixed whitespace
* Update DataViewEnumerable.cs
* Update FilterRowsByColumn.cs
Fixed extra carriage returns
* Update FilterRowsByColumn.tt
Fixed extra carriage returns
* Update FilterRowsByColumn.cs
* Update FilterRowsByColumn.tt
@ghostghost locked as resolved and limited conversation to collaborators Mar 21, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@artidoroartidoroAwaiting requested review from artidoro
@codemzscodemzsAwaiting requested review from codemzs
@eerhardteerhardtAwaiting requested review from eerhardt
@wschinwschinAwaiting requested review from wschin
2 more reviewers
@justinormontjustinormontjustinormont left review comments
@natkenatkenatke approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@sierralee51@dnfclas@eerhardt@codemzs@natke@wschin@justinormont
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Reformatting ModelOperations and DataOperations samples to width 85 - #3923
Merged
sierralee51 merged 28 commits into
dotnet:masterfrom 
sierralee51:samples
Jun 28, 2019
Merged
Reformatting ModelOperations and DataOperations samples to width 85#3923
sierralee51 merged 28 commits into
dotnet:masterfrom 
sierralee51:samples
Conversation
@sierralee51
@sierralee51sierralee51
commented
Jun 27, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
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
@dnfclas
dnfclas
commented
Jun 27, 2019•

edited

Loading

Copy link


Copy Markdown

CLA assistant check
All CLA requirements met.
codemzs
codemzs
reviewed
Jun 27, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/ModelOperations/SaveLoadModel.cs
Outdated


 // Create a prediction engine from the model for feeding new data.
 var engine = mlContext.Model.CreatePredictionEngine<Data, Transformation>(model);
 var engine = mlContext.Model.CreatePredictionEngine<Data,
@codemzscodemzsJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@eerhardt Is it ok to break generics like this or should all of it be in the same line as its corresponding class?
@codemzscodemzsJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

CC: @natke
@sierralee51sierralee51Jun 27, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I'll fix it so that I split before the "<" in a later pull request. Thanks!
@codemzs
codemzs
requested review from
codemzs and eerhardtJune 27, 2019 17:23
@sierralee51
sierralee51
requested a review
from natkeJune 27, 2019 17:32
@eerhardt
eerhardt
commented
Jun 27, 2019

Copy link


Copy Markdown

Member
85 characters per line
I kind of think that is too small. In dotnet/spark we are using 100. What's the reasoning for 85?
eerhardt
eerhardt
reviewed
Jun 27, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/ModelOperations/SaveLoadModelFile.cs
Outdated

 // Transform.
 ITransformer model = mlContext.Transforms.Conversion.MapValueToKey(outputColumnName, inputColumnName).Fit(dataView);
 ITransformer model = mlContext.Transforms.Conversion.MapValueToKey(
 outputColumnName, inputColumnName).Fit(dataView);
@eerhardteerhardtJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

For cases like this, I would rather we break per method call, than to break between a method and its parameter list.
For example:
// If you have chained method calls, line-break each method callEnumerable.Range(0,numRows).Select(i =>i.ToString()).ToArray();
That is much more readable than:
objecto=foo.methodCall(methodCallArg1,methodCallArg2).OtherMethodCall(arg3)
eerhardt
eerhardt
reviewed
Jun 27, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Program.cs
Outdated


 internal static void RunAll()
 {
 // Samples counter.
@eerhardteerhardtJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

What's the usefulness of this comment? If those words make a better name, it would be better to rename the variable with those words, than to add a comment like this.
@sierralee51sierralee51Jun 27, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

My bad, I meant to delete that. That was for an earlier test.
@sierralee51
sierralee51
commented
Jun 27, 2019

Copy link


Copy Markdown

ContributorAuthor
85 characters per line
I kind of think that is too small. In dotnet/spark we are using 100. What's the reasoning for 85?
@natke asked for 85 characters to ensure that a horizontal scrollbar is not needed on the docs webpage.
@eerhardt
eerhardt
commented
Jun 27, 2019

Copy link


Copy Markdown

Member
Ah, ok then I understand why 85 characters is better, it needs to be hosted inside a page with other things around it. Thanks for the explanation.
@sierralee51sierralee51
changed the title
Reformatting ModelOperations samples to width 85Reformatting ModelOperations and DataOperations samples to width 85Jun 27, 2019
@sierralee51
sierralee51
commented
Jun 28, 2019

Copy link


Copy Markdown

ContributorAuthor
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
*Updated to include DataOperations samples as well
justinormont
justinormont
reviewed
Jun 28, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/Cache.cs
Outdated

sierralee51and others
added 2 commits
June 28, 2019 05:42
@sierralee51@justinormont
Update docs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/Cache.cs
10f40fd
Co-Authored-By: Justin Ormont <justinormont@users.noreply.github.com>
@sierralee51
Update Program.cs
00acf2a
Got rid of test comment
@codemzs
codemzs
requested review from
eerhardt and wschinJune 28, 2019 17:14
natke
natke
reviewed
Jun 28, 2019
View reviewed changes
@natkenatke
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Width of lines looks good.
Looks like there are quite a few places with extra whitespace at the beginning of the line
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/DataViewEnumerable.cs
Outdated

Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/DataViewEnumerable.cs
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/DataViewEnumerable.tt
Outdated

 var rowEnumerableIgnoreMissing = mlContext.Data.CreateEnumerable<SampleTemperatureDataWithLatitude>(data,
 reuseRowObject: true, ignoreMissingColumns: true);
 // We can now examine the records in the IDataView. We first create an
 // enumerable of rows in the IDataView.
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra whitespace
@sierralee51sierralee51Jun 28, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Should be fixed. Some tt files have certain indentations that insure that the comments in the cs files align.
@sierralee51
Update DataViewEnumerable.tt
acdf665
Fixed extra whitespace
@sierralee51
Update DataViewEnumerable.cs
842765f
Fixed extra whitespace
@sierralee51
Update DataViewEnumerable.tt
fe9c188
Fixed extra whitespace
@sierralee51
Update FilterRowsByColumn.tt
2f91a6d
Fixed whitespace
natke
natke
approved these changes
Jun 28, 2019
View reviewed changes
@natkenatke
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Looks good! A couple of places where there is an extra carriage return
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/FilterRowsByColumn.cs
Outdated

 {
 Console.WriteLine($"{row.Date.ToString("d")}\t{row.Temperature}");
 Console.WriteLine(
 $"{row.Date.ToString("d")}\t{row.Temperature}");
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra carriage return here?
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/FilterRowsByColumn.cs
 foreach (var row in enumerable)
 {
 Console.WriteLine($"{row.Date.ToString("d")}\t{row.Temperature}");
 Console.WriteLine(
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra carriage return?
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/FilterRowsByColumn.tt
Outdated

 foreach (var row in enumerableOfData)
 {
 Console.WriteLine($""{row.Date.ToString(""d"")}\t{row.Temperature}"");
 Console.WriteLine(
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra carriage return?
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/FilterRowsByColumn.tt
 foreach (var row in enumerable)
 {
 Console.WriteLine($""{row.Date.ToString(""d"")}\t{row.Temperature}"");
 Console.WriteLine(
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra carriage return?
@sierralee51
sierralee51
merged commit 17c9155
into
dotnet:masterJun 28, 2019
Dmitry-A pushed a commit
to Dmitry-A/machinelearning
that referenced
this pull request
Jul 24, 2019
@sierralee51@Dmitry-A
Reformatting ModelOperations and DataOperations samples to width 85 (d…
b31835d
…otnet#3923)
* Added a comment.
* reformatted ModelOperations samples to width 85
* Fixed commented-on parts of MachineOperations & reformatted DataOperations
* Update docs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/Cache.cs
Co-Authored-By: Justin Ormont <justinormont@users.noreply.github.com>
* Update Program.cs
Got rid of test comment
* Update DataViewEnumerable.tt
Fixed extra whitespace
* Update DataViewEnumerable.cs
Fixed extra whitespace
* Update DataViewEnumerable.tt
Fixed extra whitespace
* Update FilterRowsByColumn.tt
Fixed whitespace
* Update ShuffleRows.tt
Fixed whitespace
* Update TakeRows.tt
Fixed whitespace
* Update TakeRows.cs
Fixed whitespace
* Update SkipRows.cs
Fixed whitespace
* Update SkipRows.tt
Fixed whitespace
* Update ShuffleRows.cs
Fixed whitespace
* Update ShuffleRows.cs
Fixed whitespace
* Update ShuffleRows.cs
* Update ShuffleRows.tt
* Update SkipRows.tt
* Update SkipRows.cs
* Update FilterRowsByColumn.cs
Fixed whitespace
* Update FilterRowsByColumn.cs
Fixed whitespace
* Update DataViewEnumerable.cs
* Update FilterRowsByColumn.cs
Fixed extra carriage returns
* Update FilterRowsByColumn.tt
Fixed extra carriage returns
* Update FilterRowsByColumn.cs
* Update FilterRowsByColumn.tt
@ghostghost
locked as resolved and limited conversation to collaborators
Mar 21, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@artidoroartidoroAwaiting requested review from artidoro
@codemzscodemzsAwaiting requested review from codemzs
@eerhardteerhardtAwaiting requested review from eerhardt
@wschinwschinAwaiting requested review from wschin

2 more reviewers

@justinormontjustinormontjustinormont left review comments
@natkenatkenatke approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

7 participants

@sierralee51@dnfclas@eerhardt@codemzs@natke@wschin@justinormont

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Reformatting ModelOperations and DataOperations samples to width 85 - #3923
Merged
sierralee51 merged 28 commits into
dotnet:masterfrom 
sierralee51:samples
Jun 28, 2019
Merged
Reformatting ModelOperations and DataOperations samples to width 85#3923
sierralee51 merged 28 commits into
dotnet:masterfrom 
sierralee51:samples
Conversation
@sierralee51
@sierralee51sierralee51
commented
Jun 27, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
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
@dnfclas
dnfclas
commented
Jun 27, 2019•

edited

Loading

Copy link


Copy Markdown

CLA assistant check
All CLA requirements met.
codemzs
codemzs
reviewed
Jun 27, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/ModelOperations/SaveLoadModel.cs
Outdated


 // Create a prediction engine from the model for feeding new data.
 var engine = mlContext.Model.CreatePredictionEngine<Data, Transformation>(model);
 var engine = mlContext.Model.CreatePredictionEngine<Data,
@codemzscodemzsJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@eerhardt Is it ok to break generics like this or should all of it be in the same line as its corresponding class?
@codemzscodemzsJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

CC: @natke
@sierralee51sierralee51Jun 27, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I'll fix it so that I split before the "<" in a later pull request. Thanks!
@codemzs
codemzs
requested review from
codemzs and eerhardtJune 27, 2019 17:23
@sierralee51
sierralee51
requested a review
from natkeJune 27, 2019 17:32
@eerhardt
eerhardt
commented
Jun 27, 2019

Copy link


Copy Markdown

Member
85 characters per line
I kind of think that is too small. In dotnet/spark we are using 100. What's the reasoning for 85?
eerhardt
eerhardt
reviewed
Jun 27, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/ModelOperations/SaveLoadModelFile.cs
Outdated

 // Transform.
 ITransformer model = mlContext.Transforms.Conversion.MapValueToKey(outputColumnName, inputColumnName).Fit(dataView);
 ITransformer model = mlContext.Transforms.Conversion.MapValueToKey(
 outputColumnName, inputColumnName).Fit(dataView);
@eerhardteerhardtJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

For cases like this, I would rather we break per method call, than to break between a method and its parameter list.
For example:
// If you have chained method calls, line-break each method callEnumerable.Range(0,numRows).Select(i =>i.ToString()).ToArray();
That is much more readable than:
objecto=foo.methodCall(methodCallArg1,methodCallArg2).OtherMethodCall(arg3)
eerhardt
eerhardt
reviewed
Jun 27, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Program.cs
Outdated


 internal static void RunAll()
 {
 // Samples counter.
@eerhardteerhardtJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

What's the usefulness of this comment? If those words make a better name, it would be better to rename the variable with those words, than to add a comment like this.
@sierralee51sierralee51Jun 27, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

My bad, I meant to delete that. That was for an earlier test.
@sierralee51
sierralee51
commented
Jun 27, 2019

Copy link


Copy Markdown

ContributorAuthor
85 characters per line
I kind of think that is too small. In dotnet/spark we are using 100. What's the reasoning for 85?
@natke asked for 85 characters to ensure that a horizontal scrollbar is not needed on the docs webpage.
@eerhardt
eerhardt
commented
Jun 27, 2019

Copy link


Copy Markdown

Member
Ah, ok then I understand why 85 characters is better, it needs to be hosted inside a page with other things around it. Thanks for the explanation.
@sierralee51sierralee51
changed the title
Reformatting ModelOperations samples to width 85Reformatting ModelOperations and DataOperations samples to width 85Jun 27, 2019
@sierralee51
sierralee51
commented
Jun 28, 2019

Copy link


Copy Markdown

ContributorAuthor
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
*Updated to include DataOperations samples as well
justinormont
justinormont
reviewed
Jun 28, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/Cache.cs
Outdated

sierralee51and others
added 2 commits
June 28, 2019 05:42
@sierralee51@justinormont
Update docs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/Cache.cs
10f40fd
Co-Authored-By: Justin Ormont <justinormont@users.noreply.github.com>
@sierralee51
Update Program.cs
00acf2a
Got rid of test comment
@codemzs
codemzs
requested review from
eerhardt and wschinJune 28, 2019 17:14
natke
natke
reviewed
Jun 28, 2019
View reviewed changes
@natkenatke
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Width of lines looks good.
Looks like there are quite a few places with extra whitespace at the beginning of the line
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/DataViewEnumerable.cs
Outdated

Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/DataViewEnumerable.cs
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/DataViewEnumerable.tt
Outdated

 var rowEnumerableIgnoreMissing = mlContext.Data.CreateEnumerable<SampleTemperatureDataWithLatitude>(data,
 reuseRowObject: true, ignoreMissingColumns: true);
 // We can now examine the records in the IDataView. We first create an
 // enumerable of rows in the IDataView.
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra whitespace
@sierralee51sierralee51Jun 28, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Should be fixed. Some tt files have certain indentations that insure that the comments in the cs files align.
@sierralee51
Update DataViewEnumerable.tt
acdf665
Fixed extra whitespace
@sierralee51
Update DataViewEnumerable.cs
842765f
Fixed extra whitespace
@sierralee51
Update DataViewEnumerable.tt
fe9c188
Fixed extra whitespace
@sierralee51
Update FilterRowsByColumn.tt
2f91a6d
Fixed whitespace
natke
natke
approved these changes
Jun 28, 2019
View reviewed changes
@natkenatke
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Looks good! A couple of places where there is an extra carriage return
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/FilterRowsByColumn.cs
Outdated

 {
 Console.WriteLine($"{row.Date.ToString("d")}\t{row.Temperature}");
 Console.WriteLine(
 $"{row.Date.ToString("d")}\t{row.Temperature}");
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra carriage return here?
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/FilterRowsByColumn.cs
 foreach (var row in enumerable)
 {
 Console.WriteLine($"{row.Date.ToString("d")}\t{row.Temperature}");
 Console.WriteLine(
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra carriage return?
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/FilterRowsByColumn.tt
Outdated

 foreach (var row in enumerableOfData)
 {
 Console.WriteLine($""{row.Date.ToString(""d"")}\t{row.Temperature}"");
 Console.WriteLine(
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra carriage return?
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/FilterRowsByColumn.tt
 foreach (var row in enumerable)
 {
 Console.WriteLine($""{row.Date.ToString(""d"")}\t{row.Temperature}"");
 Console.WriteLine(
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra carriage return?
@sierralee51
sierralee51
merged commit 17c9155
into
dotnet:masterJun 28, 2019
Dmitry-A pushed a commit
to Dmitry-A/machinelearning
that referenced
this pull request
Jul 24, 2019
@sierralee51@Dmitry-A
Reformatting ModelOperations and DataOperations samples to width 85 (d…
b31835d
…otnet#3923)
* Added a comment.
* reformatted ModelOperations samples to width 85
* Fixed commented-on parts of MachineOperations & reformatted DataOperations
* Update docs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/Cache.cs
Co-Authored-By: Justin Ormont <justinormont@users.noreply.github.com>
* Update Program.cs
Got rid of test comment
* Update DataViewEnumerable.tt
Fixed extra whitespace
* Update DataViewEnumerable.cs
Fixed extra whitespace
* Update DataViewEnumerable.tt
Fixed extra whitespace
* Update FilterRowsByColumn.tt
Fixed whitespace
* Update ShuffleRows.tt
Fixed whitespace
* Update TakeRows.tt
Fixed whitespace
* Update TakeRows.cs
Fixed whitespace
* Update SkipRows.cs
Fixed whitespace
* Update SkipRows.tt
Fixed whitespace
* Update ShuffleRows.cs
Fixed whitespace
* Update ShuffleRows.cs
Fixed whitespace
* Update ShuffleRows.cs
* Update ShuffleRows.tt
* Update SkipRows.tt
* Update SkipRows.cs
* Update FilterRowsByColumn.cs
Fixed whitespace
* Update FilterRowsByColumn.cs
Fixed whitespace
* Update DataViewEnumerable.cs
* Update FilterRowsByColumn.cs
Fixed extra carriage returns
* Update FilterRowsByColumn.tt
Fixed extra carriage returns
* Update FilterRowsByColumn.cs
* Update FilterRowsByColumn.tt
@ghostghost
locked as resolved and limited conversation to collaborators
Mar 21, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@artidoroartidoroAwaiting requested review from artidoro
@codemzscodemzsAwaiting requested review from codemzs
@eerhardteerhardtAwaiting requested review from eerhardt
@wschinwschinAwaiting requested review from wschin

2 more reviewers

@justinormontjustinormontjustinormont left review comments
@natkenatkenatke approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

7 participants

@sierralee51@dnfclas@eerhardt@codemzs@natke@wschin@justinormont

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Reformatting ModelOperations and DataOperations samples to width 85 - #3923
Merged
sierralee51 merged 28 commits into
dotnet:masterfrom 
sierralee51:samples
Jun 28, 2019
Merged
Reformatting ModelOperations and DataOperations samples to width 85#3923
sierralee51 merged 28 commits into
dotnet:masterfrom 
sierralee51:samples
Conversation
@sierralee51
@sierralee51sierralee51
commented
Jun 27, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
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
@dnfclas
dnfclas
commented
Jun 27, 2019•

edited

Loading

Copy link


Copy Markdown

CLA assistant check
All CLA requirements met.
codemzs
codemzs
reviewed
Jun 27, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/ModelOperations/SaveLoadModel.cs
Outdated


 // Create a prediction engine from the model for feeding new data.
 var engine = mlContext.Model.CreatePredictionEngine<Data, Transformation>(model);
 var engine = mlContext.Model.CreatePredictionEngine<Data,
@codemzscodemzsJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@eerhardt Is it ok to break generics like this or should all of it be in the same line as its corresponding class?
@codemzscodemzsJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

CC: @natke
@sierralee51sierralee51Jun 27, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I'll fix it so that I split before the "<" in a later pull request. Thanks!
@codemzs
codemzs
requested review from
codemzs and eerhardtJune 27, 2019 17:23
@sierralee51
sierralee51
requested a review
from natkeJune 27, 2019 17:32
@eerhardt
eerhardt
commented
Jun 27, 2019

Copy link


Copy Markdown

Member
85 characters per line
I kind of think that is too small. In dotnet/spark we are using 100. What's the reasoning for 85?
eerhardt
eerhardt
reviewed
Jun 27, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/ModelOperations/SaveLoadModelFile.cs
Outdated

 // Transform.
 ITransformer model = mlContext.Transforms.Conversion.MapValueToKey(outputColumnName, inputColumnName).Fit(dataView);
 ITransformer model = mlContext.Transforms.Conversion.MapValueToKey(
 outputColumnName, inputColumnName).Fit(dataView);
@eerhardteerhardtJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

For cases like this, I would rather we break per method call, than to break between a method and its parameter list.
For example:
// If you have chained method calls, line-break each method callEnumerable.Range(0,numRows).Select(i =>i.ToString()).ToArray();
That is much more readable than:
objecto=foo.methodCall(methodCallArg1,methodCallArg2).OtherMethodCall(arg3)
eerhardt
eerhardt
reviewed
Jun 27, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Program.cs
Outdated


 internal static void RunAll()
 {
 // Samples counter.
@eerhardteerhardtJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

What's the usefulness of this comment? If those words make a better name, it would be better to rename the variable with those words, than to add a comment like this.
@sierralee51sierralee51Jun 27, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

My bad, I meant to delete that. That was for an earlier test.
@sierralee51
sierralee51
commented
Jun 27, 2019

Copy link


Copy Markdown

ContributorAuthor
85 characters per line
I kind of think that is too small. In dotnet/spark we are using 100. What's the reasoning for 85?
@natke asked for 85 characters to ensure that a horizontal scrollbar is not needed on the docs webpage.
@eerhardt
eerhardt
commented
Jun 27, 2019

Copy link


Copy Markdown

Member
Ah, ok then I understand why 85 characters is better, it needs to be hosted inside a page with other things around it. Thanks for the explanation.
@sierralee51sierralee51
changed the title
Reformatting ModelOperations samples to width 85Reformatting ModelOperations and DataOperations samples to width 85Jun 27, 2019
@sierralee51
sierralee51
commented
Jun 28, 2019

Copy link


Copy Markdown

ContributorAuthor
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
*Updated to include DataOperations samples as well
justinormont
justinormont
reviewed
Jun 28, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/Cache.cs
Outdated

sierralee51and others
added 2 commits
June 28, 2019 05:42
@sierralee51@justinormont
Update docs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/Cache.cs
10f40fd
Co-Authored-By: Justin Ormont <justinormont@users.noreply.github.com>
@sierralee51
Update Program.cs
00acf2a
Got rid of test comment
@codemzs
codemzs
requested review from
eerhardt and wschinJune 28, 2019 17:14
natke
natke
reviewed
Jun 28, 2019
View reviewed changes
@natkenatke
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Width of lines looks good.
Looks like there are quite a few places with extra whitespace at the beginning of the line
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/DataViewEnumerable.cs
Outdated

Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/DataViewEnumerable.cs
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/DataViewEnumerable.tt
Outdated

 var rowEnumerableIgnoreMissing = mlContext.Data.CreateEnumerable<SampleTemperatureDataWithLatitude>(data,
 reuseRowObject: true, ignoreMissingColumns: true);
 // We can now examine the records in the IDataView. We first create an
 // enumerable of rows in the IDataView.
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra whitespace
@sierralee51sierralee51Jun 28, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Should be fixed. Some tt files have certain indentations that insure that the comments in the cs files align.
@sierralee51
Update DataViewEnumerable.tt
acdf665
Fixed extra whitespace
@sierralee51
Update DataViewEnumerable.cs
842765f
Fixed extra whitespace
@sierralee51
Update DataViewEnumerable.tt
fe9c188
Fixed extra whitespace
@sierralee51
Update FilterRowsByColumn.tt
2f91a6d
Fixed whitespace
natke
natke
approved these changes
Jun 28, 2019
View reviewed changes
@natkenatke
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Looks good! A couple of places where there is an extra carriage return
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/FilterRowsByColumn.cs
Outdated

 {
 Console.WriteLine($"{row.Date.ToString("d")}\t{row.Temperature}");
 Console.WriteLine(
 $"{row.Date.ToString("d")}\t{row.Temperature}");
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra carriage return here?
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/FilterRowsByColumn.cs
 foreach (var row in enumerable)
 {
 Console.WriteLine($"{row.Date.ToString("d")}\t{row.Temperature}");
 Console.WriteLine(
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra carriage return?
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/FilterRowsByColumn.tt
Outdated

 foreach (var row in enumerableOfData)
 {
 Console.WriteLine($""{row.Date.ToString(""d"")}\t{row.Temperature}"");
 Console.WriteLine(
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra carriage return?
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/FilterRowsByColumn.tt
 foreach (var row in enumerable)
 {
 Console.WriteLine($""{row.Date.ToString(""d"")}\t{row.Temperature}"");
 Console.WriteLine(
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra carriage return?
@sierralee51
sierralee51
merged commit 17c9155
into
dotnet:masterJun 28, 2019
Dmitry-A pushed a commit
to Dmitry-A/machinelearning
that referenced
this pull request
Jul 24, 2019
@sierralee51@Dmitry-A
Reformatting ModelOperations and DataOperations samples to width 85 (d…
b31835d
…otnet#3923)
* Added a comment.
* reformatted ModelOperations samples to width 85
* Fixed commented-on parts of MachineOperations & reformatted DataOperations
* Update docs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/Cache.cs
Co-Authored-By: Justin Ormont <justinormont@users.noreply.github.com>
* Update Program.cs
Got rid of test comment
* Update DataViewEnumerable.tt
Fixed extra whitespace
* Update DataViewEnumerable.cs
Fixed extra whitespace
* Update DataViewEnumerable.tt
Fixed extra whitespace
* Update FilterRowsByColumn.tt
Fixed whitespace
* Update ShuffleRows.tt
Fixed whitespace
* Update TakeRows.tt
Fixed whitespace
* Update TakeRows.cs
Fixed whitespace
* Update SkipRows.cs
Fixed whitespace
* Update SkipRows.tt
Fixed whitespace
* Update ShuffleRows.cs
Fixed whitespace
* Update ShuffleRows.cs
Fixed whitespace
* Update ShuffleRows.cs
* Update ShuffleRows.tt
* Update SkipRows.tt
* Update SkipRows.cs
* Update FilterRowsByColumn.cs
Fixed whitespace
* Update FilterRowsByColumn.cs
Fixed whitespace
* Update DataViewEnumerable.cs
* Update FilterRowsByColumn.cs
Fixed extra carriage returns
* Update FilterRowsByColumn.tt
Fixed extra carriage returns
* Update FilterRowsByColumn.cs
* Update FilterRowsByColumn.tt
@ghostghost
locked as resolved and limited conversation to collaborators
Mar 21, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@artidoroartidoroAwaiting requested review from artidoro
@codemzscodemzsAwaiting requested review from codemzs
@eerhardteerhardtAwaiting requested review from eerhardt
@wschinwschinAwaiting requested review from wschin

2 more reviewers

@justinormontjustinormontjustinormont left review comments
@natkenatkenatke approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

7 participants

@sierralee51@dnfclas@eerhardt@codemzs@natke@wschin@justinormont

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Reformatting ModelOperations and DataOperations samples to width 85 - #3923
Merged
sierralee51 merged 28 commits into
dotnet:masterfrom 
sierralee51:samples
Jun 28, 2019
Merged
Reformatting ModelOperations and DataOperations samples to width 85#3923
sierralee51 merged 28 commits into
dotnet:masterfrom 
sierralee51:samples
Conversation
@sierralee51
@sierralee51sierralee51
commented
Jun 27, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
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
@dnfclas
dnfclas
commented
Jun 27, 2019•

edited

Loading

Copy link


Copy Markdown

CLA assistant check
All CLA requirements met.
codemzs
codemzs
reviewed
Jun 27, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/ModelOperations/SaveLoadModel.cs
Outdated


 // Create a prediction engine from the model for feeding new data.
 var engine = mlContext.Model.CreatePredictionEngine<Data, Transformation>(model);
 var engine = mlContext.Model.CreatePredictionEngine<Data,
@codemzscodemzsJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@eerhardt Is it ok to break generics like this or should all of it be in the same line as its corresponding class?
@codemzscodemzsJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

CC: @natke
@sierralee51sierralee51Jun 27, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I'll fix it so that I split before the "<" in a later pull request. Thanks!
@codemzs
codemzs
requested review from
codemzs and eerhardtJune 27, 2019 17:23
@sierralee51
sierralee51
requested a review
from natkeJune 27, 2019 17:32
@eerhardt
eerhardt
commented
Jun 27, 2019

Copy link


Copy Markdown

Member
85 characters per line
I kind of think that is too small. In dotnet/spark we are using 100. What's the reasoning for 85?
eerhardt
eerhardt
reviewed
Jun 27, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/ModelOperations/SaveLoadModelFile.cs
Outdated

 // Transform.
 ITransformer model = mlContext.Transforms.Conversion.MapValueToKey(outputColumnName, inputColumnName).Fit(dataView);
 ITransformer model = mlContext.Transforms.Conversion.MapValueToKey(
 outputColumnName, inputColumnName).Fit(dataView);
@eerhardteerhardtJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

For cases like this, I would rather we break per method call, than to break between a method and its parameter list.
For example:
// If you have chained method calls, line-break each method callEnumerable.Range(0,numRows).Select(i =>i.ToString()).ToArray();
That is much more readable than:
objecto=foo.methodCall(methodCallArg1,methodCallArg2).OtherMethodCall(arg3)
eerhardt
eerhardt
reviewed
Jun 27, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Program.cs
Outdated


 internal static void RunAll()
 {
 // Samples counter.
@eerhardteerhardtJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

What's the usefulness of this comment? If those words make a better name, it would be better to rename the variable with those words, than to add a comment like this.
@sierralee51sierralee51Jun 27, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

My bad, I meant to delete that. That was for an earlier test.
@sierralee51
sierralee51
commented
Jun 27, 2019

Copy link


Copy Markdown

ContributorAuthor
85 characters per line
I kind of think that is too small. In dotnet/spark we are using 100. What's the reasoning for 85?
@natke asked for 85 characters to ensure that a horizontal scrollbar is not needed on the docs webpage.
@eerhardt
eerhardt
commented
Jun 27, 2019

Copy link


Copy Markdown

Member
Ah, ok then I understand why 85 characters is better, it needs to be hosted inside a page with other things around it. Thanks for the explanation.
@sierralee51sierralee51
changed the title
Reformatting ModelOperations samples to width 85Reformatting ModelOperations and DataOperations samples to width 85Jun 27, 2019
@sierralee51
sierralee51
commented
Jun 28, 2019

Copy link


Copy Markdown

ContributorAuthor
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
*Updated to include DataOperations samples as well
justinormont
justinormont
reviewed
Jun 28, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/Cache.cs
Outdated

sierralee51and others
added 2 commits
June 28, 2019 05:42
@sierralee51@justinormont
Update docs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/Cache.cs
10f40fd
Co-Authored-By: Justin Ormont <justinormont@users.noreply.github.com>
@sierralee51
Update Program.cs
00acf2a
Got rid of test comment
@codemzs
codemzs
requested review from
eerhardt and wschinJune 28, 2019 17:14
natke
natke
reviewed
Jun 28, 2019
View reviewed changes
@natkenatke
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Width of lines looks good.
Looks like there are quite a few places with extra whitespace at the beginning of the line
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/DataViewEnumerable.cs
Outdated

Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/DataViewEnumerable.cs
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/DataViewEnumerable.tt
Outdated

 var rowEnumerableIgnoreMissing = mlContext.Data.CreateEnumerable<SampleTemperatureDataWithLatitude>(data,
 reuseRowObject: true, ignoreMissingColumns: true);
 // We can now examine the records in the IDataView. We first create an
 // enumerable of rows in the IDataView.
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra whitespace
@sierralee51sierralee51Jun 28, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Should be fixed. Some tt files have certain indentations that insure that the comments in the cs files align.
@sierralee51
Update DataViewEnumerable.tt
acdf665
Fixed extra whitespace
@sierralee51
Update DataViewEnumerable.cs
842765f
Fixed extra whitespace
@sierralee51
Update DataViewEnumerable.tt
fe9c188
Fixed extra whitespace
@sierralee51
Update FilterRowsByColumn.tt
2f91a6d
Fixed whitespace
natke
natke
approved these changes
Jun 28, 2019
View reviewed changes
@natkenatke
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Looks good! A couple of places where there is an extra carriage return
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/FilterRowsByColumn.cs
Outdated

 {
 Console.WriteLine($"{row.Date.ToString("d")}\t{row.Temperature}");
 Console.WriteLine(
 $"{row.Date.ToString("d")}\t{row.Temperature}");
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra carriage return here?
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/FilterRowsByColumn.cs
 foreach (var row in enumerable)
 {
 Console.WriteLine($"{row.Date.ToString("d")}\t{row.Temperature}");
 Console.WriteLine(
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra carriage return?
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/FilterRowsByColumn.tt
Outdated

 foreach (var row in enumerableOfData)
 {
 Console.WriteLine($""{row.Date.ToString(""d"")}\t{row.Temperature}"");
 Console.WriteLine(
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra carriage return?
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/FilterRowsByColumn.tt
 foreach (var row in enumerable)
 {
 Console.WriteLine($""{row.Date.ToString(""d"")}\t{row.Temperature}"");
 Console.WriteLine(
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra carriage return?
@sierralee51
sierralee51
merged commit 17c9155
into
dotnet:masterJun 28, 2019
Dmitry-A pushed a commit
to Dmitry-A/machinelearning
that referenced
this pull request
Jul 24, 2019
@sierralee51@Dmitry-A
Reformatting ModelOperations and DataOperations samples to width 85 (d…
b31835d
…otnet#3923)
* Added a comment.
* reformatted ModelOperations samples to width 85
* Fixed commented-on parts of MachineOperations & reformatted DataOperations
* Update docs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/Cache.cs
Co-Authored-By: Justin Ormont <justinormont@users.noreply.github.com>
* Update Program.cs
Got rid of test comment
* Update DataViewEnumerable.tt
Fixed extra whitespace
* Update DataViewEnumerable.cs
Fixed extra whitespace
* Update DataViewEnumerable.tt
Fixed extra whitespace
* Update FilterRowsByColumn.tt
Fixed whitespace
* Update ShuffleRows.tt
Fixed whitespace
* Update TakeRows.tt
Fixed whitespace
* Update TakeRows.cs
Fixed whitespace
* Update SkipRows.cs
Fixed whitespace
* Update SkipRows.tt
Fixed whitespace
* Update ShuffleRows.cs
Fixed whitespace
* Update ShuffleRows.cs
Fixed whitespace
* Update ShuffleRows.cs
* Update ShuffleRows.tt
* Update SkipRows.tt
* Update SkipRows.cs
* Update FilterRowsByColumn.cs
Fixed whitespace
* Update FilterRowsByColumn.cs
Fixed whitespace
* Update DataViewEnumerable.cs
* Update FilterRowsByColumn.cs
Fixed extra carriage returns
* Update FilterRowsByColumn.tt
Fixed extra carriage returns
* Update FilterRowsByColumn.cs
* Update FilterRowsByColumn.tt
@ghostghost
locked as resolved and limited conversation to collaborators
Mar 21, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@artidoroartidoroAwaiting requested review from artidoro
@codemzscodemzsAwaiting requested review from codemzs
@eerhardteerhardtAwaiting requested review from eerhardt
@wschinwschinAwaiting requested review from wschin

2 more reviewers

@justinormontjustinormontjustinormont left review comments
@natkenatkenatke approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

7 participants

@sierralee51@dnfclas@eerhardt@codemzs@natke@wschin@justinormont

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Reformatting ModelOperations and DataOperations samples to width 85 - #3923
Merged
sierralee51 merged 28 commits into
dotnet:masterfrom 
sierralee51:samples
Jun 28, 2019
Merged
Reformatting ModelOperations and DataOperations samples to width 85#3923
sierralee51 merged 28 commits into
dotnet:masterfrom 
sierralee51:samples
Conversation
@sierralee51
@sierralee51sierralee51
commented
Jun 27, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
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
@dnfclas
dnfclas
commented
Jun 27, 2019•

edited

Loading

Copy link


Copy Markdown

CLA assistant check
All CLA requirements met.
codemzs
codemzs
reviewed
Jun 27, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/ModelOperations/SaveLoadModel.cs
Outdated


 // Create a prediction engine from the model for feeding new data.
 var engine = mlContext.Model.CreatePredictionEngine<Data, Transformation>(model);
 var engine = mlContext.Model.CreatePredictionEngine<Data,
@codemzscodemzsJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@eerhardt Is it ok to break generics like this or should all of it be in the same line as its corresponding class?
@codemzscodemzsJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

CC: @natke
@sierralee51sierralee51Jun 27, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I'll fix it so that I split before the "<" in a later pull request. Thanks!
@codemzs
codemzs
requested review from
codemzs and eerhardtJune 27, 2019 17:23
@sierralee51
sierralee51
requested a review
from natkeJune 27, 2019 17:32
@eerhardt
eerhardt
commented
Jun 27, 2019

Copy link


Copy Markdown

Member
85 characters per line
I kind of think that is too small. In dotnet/spark we are using 100. What's the reasoning for 85?
eerhardt
eerhardt
reviewed
Jun 27, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/ModelOperations/SaveLoadModelFile.cs
Outdated

 // Transform.
 ITransformer model = mlContext.Transforms.Conversion.MapValueToKey(outputColumnName, inputColumnName).Fit(dataView);
 ITransformer model = mlContext.Transforms.Conversion.MapValueToKey(
 outputColumnName, inputColumnName).Fit(dataView);
@eerhardteerhardtJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

For cases like this, I would rather we break per method call, than to break between a method and its parameter list.
For example:
// If you have chained method calls, line-break each method callEnumerable.Range(0,numRows).Select(i =>i.ToString()).ToArray();
That is much more readable than:
objecto=foo.methodCall(methodCallArg1,methodCallArg2).OtherMethodCall(arg3)
eerhardt
eerhardt
reviewed
Jun 27, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Program.cs
Outdated


 internal static void RunAll()
 {
 // Samples counter.
@eerhardteerhardtJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

What's the usefulness of this comment? If those words make a better name, it would be better to rename the variable with those words, than to add a comment like this.
@sierralee51sierralee51Jun 27, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

My bad, I meant to delete that. That was for an earlier test.
@sierralee51
sierralee51
commented
Jun 27, 2019

Copy link


Copy Markdown

ContributorAuthor
85 characters per line
I kind of think that is too small. In dotnet/spark we are using 100. What's the reasoning for 85?
@natke asked for 85 characters to ensure that a horizontal scrollbar is not needed on the docs webpage.
@eerhardt
eerhardt
commented
Jun 27, 2019

Copy link


Copy Markdown

Member
Ah, ok then I understand why 85 characters is better, it needs to be hosted inside a page with other things around it. Thanks for the explanation.
@sierralee51sierralee51
changed the title
Reformatting ModelOperations samples to width 85Reformatting ModelOperations and DataOperations samples to width 85Jun 27, 2019
@sierralee51
sierralee51
commented
Jun 28, 2019

Copy link


Copy Markdown

ContributorAuthor
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
*Updated to include DataOperations samples as well
justinormont
justinormont
reviewed
Jun 28, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/Cache.cs
Outdated

sierralee51and others
added 2 commits
June 28, 2019 05:42
@sierralee51@justinormont
Update docs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/Cache.cs
10f40fd
Co-Authored-By: Justin Ormont <justinormont@users.noreply.github.com>
@sierralee51
Update Program.cs
00acf2a
Got rid of test comment
@codemzs
codemzs
requested review from
eerhardt and wschinJune 28, 2019 17:14
natke
natke
reviewed
Jun 28, 2019
View reviewed changes
@natkenatke
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Width of lines looks good.
Looks like there are quite a few places with extra whitespace at the beginning of the line
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/DataViewEnumerable.cs
Outdated

Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/DataViewEnumerable.cs
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/DataViewEnumerable.tt
Outdated

 var rowEnumerableIgnoreMissing = mlContext.Data.CreateEnumerable<SampleTemperatureDataWithLatitude>(data,
 reuseRowObject: true, ignoreMissingColumns: true);
 // We can now examine the records in the IDataView. We first create an
 // enumerable of rows in the IDataView.
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra whitespace
@sierralee51sierralee51Jun 28, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Should be fixed. Some tt files have certain indentations that insure that the comments in the cs files align.
@sierralee51
Update DataViewEnumerable.tt
acdf665
Fixed extra whitespace
@sierralee51
Update DataViewEnumerable.cs
842765f
Fixed extra whitespace
@sierralee51
Update DataViewEnumerable.tt
fe9c188
Fixed extra whitespace
@sierralee51
Update FilterRowsByColumn.tt
2f91a6d
Fixed whitespace
natke
natke
approved these changes
Jun 28, 2019
View reviewed changes
@natkenatke
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Looks good! A couple of places where there is an extra carriage return
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/FilterRowsByColumn.cs
Outdated

 {
 Console.WriteLine($"{row.Date.ToString("d")}\t{row.Temperature}");
 Console.WriteLine(
 $"{row.Date.ToString("d")}\t{row.Temperature}");
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra carriage return here?
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/FilterRowsByColumn.cs
 foreach (var row in enumerable)
 {
 Console.WriteLine($"{row.Date.ToString("d")}\t{row.Temperature}");
 Console.WriteLine(
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra carriage return?
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/FilterRowsByColumn.tt
Outdated

 foreach (var row in enumerableOfData)
 {
 Console.WriteLine($""{row.Date.ToString(""d"")}\t{row.Temperature}"");
 Console.WriteLine(
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra carriage return?
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/FilterRowsByColumn.tt
 foreach (var row in enumerable)
 {
 Console.WriteLine($""{row.Date.ToString(""d"")}\t{row.Temperature}"");
 Console.WriteLine(
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra carriage return?
@sierralee51
sierralee51
merged commit 17c9155
into
dotnet:masterJun 28, 2019
Dmitry-A pushed a commit
to Dmitry-A/machinelearning
that referenced
this pull request
Jul 24, 2019
@sierralee51@Dmitry-A
Reformatting ModelOperations and DataOperations samples to width 85 (d…
b31835d
…otnet#3923)
* Added a comment.
* reformatted ModelOperations samples to width 85
* Fixed commented-on parts of MachineOperations & reformatted DataOperations
* Update docs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/Cache.cs
Co-Authored-By: Justin Ormont <justinormont@users.noreply.github.com>
* Update Program.cs
Got rid of test comment
* Update DataViewEnumerable.tt
Fixed extra whitespace
* Update DataViewEnumerable.cs
Fixed extra whitespace
* Update DataViewEnumerable.tt
Fixed extra whitespace
* Update FilterRowsByColumn.tt
Fixed whitespace
* Update ShuffleRows.tt
Fixed whitespace
* Update TakeRows.tt
Fixed whitespace
* Update TakeRows.cs
Fixed whitespace
* Update SkipRows.cs
Fixed whitespace
* Update SkipRows.tt
Fixed whitespace
* Update ShuffleRows.cs
Fixed whitespace
* Update ShuffleRows.cs
Fixed whitespace
* Update ShuffleRows.cs
* Update ShuffleRows.tt
* Update SkipRows.tt
* Update SkipRows.cs
* Update FilterRowsByColumn.cs
Fixed whitespace
* Update FilterRowsByColumn.cs
Fixed whitespace
* Update DataViewEnumerable.cs
* Update FilterRowsByColumn.cs
Fixed extra carriage returns
* Update FilterRowsByColumn.tt
Fixed extra carriage returns
* Update FilterRowsByColumn.cs
* Update FilterRowsByColumn.tt
@ghostghost
locked as resolved and limited conversation to collaborators
Mar 21, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@artidoroartidoroAwaiting requested review from artidoro
@codemzscodemzsAwaiting requested review from codemzs
@eerhardteerhardtAwaiting requested review from eerhardt
@wschinwschinAwaiting requested review from wschin

2 more reviewers

@justinormontjustinormontjustinormont left review comments
@natkenatkenatke approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

7 participants

@sierralee51@dnfclas@eerhardt@codemzs@natke@wschin@justinormont

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Reformatting ModelOperations and DataOperations samples to width 85 - #3923
Merged
sierralee51 merged 28 commits into
dotnet:masterfrom 
sierralee51:samples
Jun 28, 2019
Merged
Reformatting ModelOperations and DataOperations samples to width 85#3923
sierralee51 merged 28 commits into
dotnet:masterfrom 
sierralee51:samples
Conversation
@sierralee51
@sierralee51sierralee51
commented
Jun 27, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
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
@dnfclas
dnfclas
commented
Jun 27, 2019•

edited

Loading

Copy link


Copy Markdown

CLA assistant check
All CLA requirements met.
codemzs
codemzs
reviewed
Jun 27, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/ModelOperations/SaveLoadModel.cs
Outdated


 // Create a prediction engine from the model for feeding new data.
 var engine = mlContext.Model.CreatePredictionEngine<Data, Transformation>(model);
 var engine = mlContext.Model.CreatePredictionEngine<Data,
@codemzscodemzsJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@eerhardt Is it ok to break generics like this or should all of it be in the same line as its corresponding class?
@codemzscodemzsJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

CC: @natke
@sierralee51sierralee51Jun 27, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I'll fix it so that I split before the "<" in a later pull request. Thanks!
@codemzs
codemzs
requested review from
codemzs and eerhardtJune 27, 2019 17:23
@sierralee51
sierralee51
requested a review
from natkeJune 27, 2019 17:32
@eerhardt
eerhardt
commented
Jun 27, 2019

Copy link


Copy Markdown

Member
85 characters per line
I kind of think that is too small. In dotnet/spark we are using 100. What's the reasoning for 85?
eerhardt
eerhardt
reviewed
Jun 27, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/ModelOperations/SaveLoadModelFile.cs
Outdated

 // Transform.
 ITransformer model = mlContext.Transforms.Conversion.MapValueToKey(outputColumnName, inputColumnName).Fit(dataView);
 ITransformer model = mlContext.Transforms.Conversion.MapValueToKey(
 outputColumnName, inputColumnName).Fit(dataView);
@eerhardteerhardtJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

For cases like this, I would rather we break per method call, than to break between a method and its parameter list.
For example:
// If you have chained method calls, line-break each method callEnumerable.Range(0,numRows).Select(i =>i.ToString()).ToArray();
That is much more readable than:
objecto=foo.methodCall(methodCallArg1,methodCallArg2).OtherMethodCall(arg3)
eerhardt
eerhardt
reviewed
Jun 27, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Program.cs
Outdated


 internal static void RunAll()
 {
 // Samples counter.
@eerhardteerhardtJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

What's the usefulness of this comment? If those words make a better name, it would be better to rename the variable with those words, than to add a comment like this.
@sierralee51sierralee51Jun 27, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

My bad, I meant to delete that. That was for an earlier test.
@sierralee51
sierralee51
commented
Jun 27, 2019

Copy link


Copy Markdown

ContributorAuthor
85 characters per line
I kind of think that is too small. In dotnet/spark we are using 100. What's the reasoning for 85?
@natke asked for 85 characters to ensure that a horizontal scrollbar is not needed on the docs webpage.
@eerhardt
eerhardt
commented
Jun 27, 2019

Copy link


Copy Markdown

Member
Ah, ok then I understand why 85 characters is better, it needs to be hosted inside a page with other things around it. Thanks for the explanation.
@sierralee51sierralee51
changed the title
Reformatting ModelOperations samples to width 85Reformatting ModelOperations and DataOperations samples to width 85Jun 27, 2019
@sierralee51
sierralee51
commented
Jun 28, 2019

Copy link


Copy Markdown

ContributorAuthor
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
*Updated to include DataOperations samples as well
justinormont
justinormont
reviewed
Jun 28, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/Cache.cs
Outdated

sierralee51and others
added 2 commits
June 28, 2019 05:42
@sierralee51@justinormont
Update docs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/Cache.cs
10f40fd
Co-Authored-By: Justin Ormont <justinormont@users.noreply.github.com>
@sierralee51
Update Program.cs
00acf2a
Got rid of test comment
@codemzs
codemzs
requested review from
eerhardt and wschinJune 28, 2019 17:14
natke
natke
reviewed
Jun 28, 2019
View reviewed changes
@natkenatke
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Width of lines looks good.
Looks like there are quite a few places with extra whitespace at the beginning of the line
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/DataViewEnumerable.cs
Outdated

Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/DataViewEnumerable.cs
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/DataViewEnumerable.tt
Outdated

 var rowEnumerableIgnoreMissing = mlContext.Data.CreateEnumerable<SampleTemperatureDataWithLatitude>(data,
 reuseRowObject: true, ignoreMissingColumns: true);
 // We can now examine the records in the IDataView. We first create an
 // enumerable of rows in the IDataView.
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra whitespace
@sierralee51sierralee51Jun 28, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Should be fixed. Some tt files have certain indentations that insure that the comments in the cs files align.
@sierralee51
Update DataViewEnumerable.tt
acdf665
Fixed extra whitespace
@sierralee51
Update DataViewEnumerable.cs
842765f
Fixed extra whitespace
@sierralee51
Update DataViewEnumerable.tt
fe9c188
Fixed extra whitespace
@sierralee51
Update FilterRowsByColumn.tt
2f91a6d
Fixed whitespace
natke
natke
approved these changes
Jun 28, 2019
View reviewed changes
@natkenatke
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Looks good! A couple of places where there is an extra carriage return
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/FilterRowsByColumn.cs
Outdated

 {
 Console.WriteLine($"{row.Date.ToString("d")}\t{row.Temperature}");
 Console.WriteLine(
 $"{row.Date.ToString("d")}\t{row.Temperature}");
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra carriage return here?
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/FilterRowsByColumn.cs
 foreach (var row in enumerable)
 {
 Console.WriteLine($"{row.Date.ToString("d")}\t{row.Temperature}");
 Console.WriteLine(
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra carriage return?
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/FilterRowsByColumn.tt
Outdated

 foreach (var row in enumerableOfData)
 {
 Console.WriteLine($""{row.Date.ToString(""d"")}\t{row.Temperature}"");
 Console.WriteLine(
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra carriage return?
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/FilterRowsByColumn.tt
 foreach (var row in enumerable)
 {
 Console.WriteLine($""{row.Date.ToString(""d"")}\t{row.Temperature}"");
 Console.WriteLine(
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra carriage return?
@sierralee51
sierralee51
merged commit 17c9155
into
dotnet:masterJun 28, 2019
Dmitry-A pushed a commit
to Dmitry-A/machinelearning
that referenced
this pull request
Jul 24, 2019
@sierralee51@Dmitry-A
Reformatting ModelOperations and DataOperations samples to width 85 (d…
b31835d
…otnet#3923)
* Added a comment.
* reformatted ModelOperations samples to width 85
* Fixed commented-on parts of MachineOperations & reformatted DataOperations
* Update docs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/Cache.cs
Co-Authored-By: Justin Ormont <justinormont@users.noreply.github.com>
* Update Program.cs
Got rid of test comment
* Update DataViewEnumerable.tt
Fixed extra whitespace
* Update DataViewEnumerable.cs
Fixed extra whitespace
* Update DataViewEnumerable.tt
Fixed extra whitespace
* Update FilterRowsByColumn.tt
Fixed whitespace
* Update ShuffleRows.tt
Fixed whitespace
* Update TakeRows.tt
Fixed whitespace
* Update TakeRows.cs
Fixed whitespace
* Update SkipRows.cs
Fixed whitespace
* Update SkipRows.tt
Fixed whitespace
* Update ShuffleRows.cs
Fixed whitespace
* Update ShuffleRows.cs
Fixed whitespace
* Update ShuffleRows.cs
* Update ShuffleRows.tt
* Update SkipRows.tt
* Update SkipRows.cs
* Update FilterRowsByColumn.cs
Fixed whitespace
* Update FilterRowsByColumn.cs
Fixed whitespace
* Update DataViewEnumerable.cs
* Update FilterRowsByColumn.cs
Fixed extra carriage returns
* Update FilterRowsByColumn.tt
Fixed extra carriage returns
* Update FilterRowsByColumn.cs
* Update FilterRowsByColumn.tt
@ghostghost
locked as resolved and limited conversation to collaborators
Mar 21, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@artidoroartidoroAwaiting requested review from artidoro
@codemzscodemzsAwaiting requested review from codemzs
@eerhardteerhardtAwaiting requested review from eerhardt
@wschinwschinAwaiting requested review from wschin

2 more reviewers

@justinormontjustinormontjustinormont left review comments
@natkenatkenatke approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

7 participants

@sierralee51@dnfclas@eerhardt@codemzs@natke@wschin@justinormont

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Reformatting ModelOperations and DataOperations samples to width 85 - #3923
Merged
sierralee51 merged 28 commits into
dotnet:masterfrom 
sierralee51:samples
Jun 28, 2019
Merged
Reformatting ModelOperations and DataOperations samples to width 85#3923
sierralee51 merged 28 commits into
dotnet:masterfrom 
sierralee51:samples
Conversation
@sierralee51
@sierralee51sierralee51
commented
Jun 27, 2019•

edited

Loading

Copy link


Copy Markdown

Contributor
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
@dnfclas
dnfclas
commented
Jun 27, 2019•

edited

Loading

Copy link


Copy Markdown

CLA assistant check
All CLA requirements met.
codemzs
codemzs
reviewed
Jun 27, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/ModelOperations/SaveLoadModel.cs
Outdated


 // Create a prediction engine from the model for feeding new data.
 var engine = mlContext.Model.CreatePredictionEngine<Data, Transformation>(model);
 var engine = mlContext.Model.CreatePredictionEngine<Data,
@codemzscodemzsJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

@eerhardt Is it ok to break generics like this or should all of it be in the same line as its corresponding class?
@codemzscodemzsJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

CC: @natke
@sierralee51sierralee51Jun 27, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

I'll fix it so that I split before the "<" in a later pull request. Thanks!
@codemzs
codemzs
requested review from
codemzs and eerhardtJune 27, 2019 17:23
@sierralee51
sierralee51
requested a review
from natkeJune 27, 2019 17:32
@eerhardt
eerhardt
commented
Jun 27, 2019

Copy link


Copy Markdown

Member
85 characters per line
I kind of think that is too small. In dotnet/spark we are using 100. What's the reasoning for 85?
eerhardt
eerhardt
reviewed
Jun 27, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/ModelOperations/SaveLoadModelFile.cs
Outdated

 // Transform.
 ITransformer model = mlContext.Transforms.Conversion.MapValueToKey(outputColumnName, inputColumnName).Fit(dataView);
 ITransformer model = mlContext.Transforms.Conversion.MapValueToKey(
 outputColumnName, inputColumnName).Fit(dataView);
@eerhardteerhardtJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

For cases like this, I would rather we break per method call, than to break between a method and its parameter list.
For example:
// If you have chained method calls, line-break each method callEnumerable.Range(0,numRows).Select(i =>i.ToString()).ToArray();
That is much more readable than:
objecto=foo.methodCall(methodCallArg1,methodCallArg2).OtherMethodCall(arg3)
eerhardt
eerhardt
reviewed
Jun 27, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Program.cs
Outdated


 internal static void RunAll()
 {
 // Samples counter.
@eerhardteerhardtJun 27, 2019

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

What's the usefulness of this comment? If those words make a better name, it would be better to rename the variable with those words, than to add a comment like this.
@sierralee51sierralee51Jun 27, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

My bad, I meant to delete that. That was for an earlier test.
@sierralee51
sierralee51
commented
Jun 27, 2019

Copy link


Copy Markdown

ContributorAuthor
85 characters per line
I kind of think that is too small. In dotnet/spark we are using 100. What's the reasoning for 85?
@natke asked for 85 characters to ensure that a horizontal scrollbar is not needed on the docs webpage.
@eerhardt
eerhardt
commented
Jun 27, 2019

Copy link


Copy Markdown

Member
Ah, ok then I understand why 85 characters is better, it needs to be hosted inside a page with other things around it. Thanks for the explanation.
@sierralee51sierralee51
changed the title
Reformatting ModelOperations samples to width 85Reformatting ModelOperations and DataOperations samples to width 85Jun 27, 2019
@sierralee51
sierralee51
commented
Jun 28, 2019

Copy link


Copy Markdown

ContributorAuthor
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
*Updated to include DataOperations samples as well
justinormont
justinormont
reviewed
Jun 28, 2019
View reviewed changes
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/Cache.cs
Outdated

sierralee51and others
added 2 commits
June 28, 2019 05:42
@sierralee51@justinormont
Update docs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/Cache.cs
10f40fd
Co-Authored-By: Justin Ormont <justinormont@users.noreply.github.com>
@sierralee51
Update Program.cs
00acf2a
Got rid of test comment
@codemzs
codemzs
requested review from
eerhardt and wschinJune 28, 2019 17:14
natke
natke
reviewed
Jun 28, 2019
View reviewed changes
@natkenatke
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Width of lines looks good.
Looks like there are quite a few places with extra whitespace at the beginning of the line
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/DataViewEnumerable.cs
Outdated

Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/DataViewEnumerable.cs
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/DataViewEnumerable.tt
Outdated

 var rowEnumerableIgnoreMissing = mlContext.Data.CreateEnumerable<SampleTemperatureDataWithLatitude>(data,
 reuseRowObject: true, ignoreMissingColumns: true);
 // We can now examine the records in the IDataView. We first create an
 // enumerable of rows in the IDataView.
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra whitespace
@sierralee51sierralee51Jun 28, 2019

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

Should be fixed. Some tt files have certain indentations that insure that the comments in the cs files align.
@sierralee51
Update DataViewEnumerable.tt
acdf665
Fixed extra whitespace
@sierralee51
Update DataViewEnumerable.cs
842765f
Fixed extra whitespace
@sierralee51
Update DataViewEnumerable.tt
fe9c188
Fixed extra whitespace
@sierralee51
Update FilterRowsByColumn.tt
2f91a6d
Fixed whitespace
natke
natke
approved these changes
Jun 28, 2019
View reviewed changes
@natkenatke
left a comment


Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Looks good! A couple of places where there is an extra carriage return
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/FilterRowsByColumn.cs
Outdated

 {
 Console.WriteLine($"{row.Date.ToString("d")}\t{row.Temperature}");
 Console.WriteLine(
 $"{row.Date.ToString("d")}\t{row.Temperature}");
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra carriage return here?
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/FilterRowsByColumn.cs
 foreach (var row in enumerable)
 {
 Console.WriteLine($"{row.Date.ToString("d")}\t{row.Temperature}");
 Console.WriteLine(
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra carriage return?
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/FilterRowsByColumn.tt
Outdated

 foreach (var row in enumerableOfData)
 {
 Console.WriteLine($""{row.Date.ToString(""d"")}\t{row.Temperature}"");
 Console.WriteLine(
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra carriage return?
Comment threaddocs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/FilterRowsByColumn.tt
 foreach (var row in enumerable)
 {
 Console.WriteLine($""{row.Date.ToString(""d"")}\t{row.Temperature}"");
 Console.WriteLine(
@natkenatkeJun 28, 2019

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Extra carriage return?
@sierralee51
sierralee51
merged commit 17c9155
into
dotnet:masterJun 28, 2019
Dmitry-A pushed a commit
to Dmitry-A/machinelearning
that referenced
this pull request
Jul 24, 2019
@sierralee51@Dmitry-A
Reformatting ModelOperations and DataOperations samples to width 85 (d…
b31835d
…otnet#3923)
* Added a comment.
* reformatted ModelOperations samples to width 85
* Fixed commented-on parts of MachineOperations & reformatted DataOperations
* Update docs/samples/Microsoft.ML.Samples/Dynamic/DataOperations/Cache.cs
Co-Authored-By: Justin Ormont <justinormont@users.noreply.github.com>
* Update Program.cs
Got rid of test comment
* Update DataViewEnumerable.tt
Fixed extra whitespace
* Update DataViewEnumerable.cs
Fixed extra whitespace
* Update DataViewEnumerable.tt
Fixed extra whitespace
* Update FilterRowsByColumn.tt
Fixed whitespace
* Update ShuffleRows.tt
Fixed whitespace
* Update TakeRows.tt
Fixed whitespace
* Update TakeRows.cs
Fixed whitespace
* Update SkipRows.cs
Fixed whitespace
* Update SkipRows.tt
Fixed whitespace
* Update ShuffleRows.cs
Fixed whitespace
* Update ShuffleRows.cs
Fixed whitespace
* Update ShuffleRows.cs
* Update ShuffleRows.tt
* Update SkipRows.tt
* Update SkipRows.cs
* Update FilterRowsByColumn.cs
Fixed whitespace
* Update FilterRowsByColumn.cs
Fixed whitespace
* Update DataViewEnumerable.cs
* Update FilterRowsByColumn.cs
Fixed extra carriage returns
* Update FilterRowsByColumn.tt
Fixed extra carriage returns
* Update FilterRowsByColumn.cs
* Update FilterRowsByColumn.tt
@ghostghost
locked as resolved and limited conversation to collaborators
Mar 21, 2022
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@artidoroartidoroAwaiting requested review from artidoro
@codemzscodemzsAwaiting requested review from codemzs
@eerhardteerhardtAwaiting requested review from eerhardt
@wschinwschinAwaiting requested review from wschin

2 more reviewers

@justinormontjustinormontjustinormont left review comments
@natkenatkenatke approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels


None yet


Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

7 participants

@sierralee51@dnfclas@eerhardt@codemzs@natke@wschin@justinormont