Reformatted Recommendation samples to width 85 - #3941

Merged
sayanshaw24 merged 3 commits into
dotnet:masterfrom
sayanshaw24:recommendationReformat
Jul 2, 2019
Merged

Reformatted Recommendation samples to width 85#3941
sayanshaw24 merged 3 commits into
dotnet:masterfrom
sayanshaw24:recommendationReformat

Conversation

@sayanshaw24

Copy link
Copy Markdown
Contributor

Guidelines followed:

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

dataMatrix.Add(new MatrixElement() { MatrixColumnIndex = i, MatrixRowIndex = j, Value = (i + j) % 5 });
dataMatrix.Add(new MatrixElement() { MatrixColumnIndex = i,
MatrixRowIndex = j, Value = (i + j) % 5 });

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.

Empty line. Is it produced by an auto-formatting tool?

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.

Actually, Zeeshan asked us to do that, so that's what we've been doing as a norm.

Console.WriteLine($"Root Mean Squared Error: {metrics.RootMeanSquaredError:F2}");
Console.WriteLine($"RSquared: {metrics.RSquared:F2}");
Console.WriteLine("Mean Absolute Error: " + metrics.MeanAbsoluteError +
":F2");

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.

Is this correct? I thought F2 tells the prcision.

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'm not sure either. I tried reformatting it like another example Zeeshan helped us with that used a similar format. Do you know what the right notation is?

":F2");

Console.WriteLine("Mean Squared Error: " + metrics.MeanSquaredError +
":F2");

@wschinwschinJul 1, 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.

Maybe merging the two lines to

Console.WriteLine("Mean Squared Error: " + metrics.MeanSquaredError);

could be a reasonable setting.

// its desired value would be set 0.15. In other words, this parameter determines
// the value of all missing matrix elements.
MatrixColumnIndexColumnName = nameof(
MatrixElement.MatrixColumnIndex),

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.

Line 46 and Lines 47-53 should be at the same indent level.

@sayanshaw24sayanshaw24Jul 2, 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.

Zeeshan asked us to indent after the first break, so we've been doing that for all the files. Maybe we can discuss this along with Zeeshan in the scrum tomorrow?

@codemzscodemzs left a comment

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.

:shipit:

@sayanshaw24
sayanshaw24 merged commit 01b3ec7 into dotnet:masterJul 2, 2019
var predictions = mlContext.Data.CreateEnumerable<MatrixElement>(transformedData, reuseRowObject: false).Take(5).ToList();
var predictions = mlContext.Data
.CreateEnumerable<MatrixElement>(transformedData,
reuseRowObject: false).Take(5).ToList();

@sharwellsharwellJul 2, 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.

📝 This layout is very difficult to read. The following would be preferable:

varpredictions=mlContext.Data.CreateEnumerable<MatrixElement>(transformedData,reuseRowObject:false).Take(5).ToList();

nameof(MatrixElement.MatrixRowIndex), 10, 0.2, 1);
var pipeline = mlContext.Recommendation().Trainers.
MatrixFactorization(nameof(MatrixElement.Value),
nameof(MatrixElement.MatrixColumnIndex),

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.

📝 This is incorrectly indented

for (uint j = 0; j < MatrixRowCount; ++j)
dataMatrix.Add(new MatrixElement() { MatrixColumnIndex = i, MatrixRowIndex = j, Value = (i + j) % 5 });
dataMatrix.Add(new MatrixElement() { MatrixColumnIndex = i,
MatrixRowIndex = j, Value = (i + j) % 5 });

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.

📝 Incorrect line wrapping and indentation

@sharwellsharwell 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.

Requesting changes. The wrapping applied in this pull request produces worse readability than whatever tool was automatically wrapping past the 85 character limit.

// the value of all missing matrix elements.
MatrixColumnIndexColumnName = nameof(
MatrixElement.MatrixColumnIndex),
MatrixRowIndexColumnName = nameof(MatrixElement.MatrixRowIndex),

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.

📝 This is incorrectly indented

// Apply the trained model to the test set. Notice that training is a
// partial
var prediction = model.Transform(mlContext.Data.LoadFromEnumerable(
testData));

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.

📝 Incorrect wrapping (prefer to wrap the outer argument when it will fit on the same number of lines)

// Specify IDataView colum which stores matrix row indexes.
// Specify IDataView colum which stores matrix column indexes.
MatrixColumnIndexColumnName = nameof(MatrixElement.MatrixColumnIndex
),

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.

📝 Incorrect wrapping (should be wrapped before nameof)

Dmitry-A pushed a commit to Dmitry-A/machinelearning that referenced this pull request Jul 24, 2019
* Reformatted Recommendation samples
* Fix for tab spacing errors
* Removed precision for reformatting in Console.WriteLine and fixed KeyType error
@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.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@sayanshaw24@codemzs@sharwell@natke@wschin
, '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

Reformatted Recommendation samples to width 85 - #3941

Merged
sayanshaw24 merged 3 commits into
dotnet:masterfrom
sayanshaw24:recommendationReformat
Jul 2, 2019
Merged

Reformatted Recommendation samples to width 85#3941
sayanshaw24 merged 3 commits into
dotnet:masterfrom
sayanshaw24:recommendationReformat

Conversation

@sayanshaw24

Copy link
Copy Markdown
Contributor

Guidelines followed:

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

dataMatrix.Add(new MatrixElement() { MatrixColumnIndex = i, MatrixRowIndex = j, Value = (i + j) % 5 });
dataMatrix.Add(new MatrixElement() { MatrixColumnIndex = i,
MatrixRowIndex = j, Value = (i + j) % 5 });

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.

Empty line. Is it produced by an auto-formatting tool?

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.

Actually, Zeeshan asked us to do that, so that's what we've been doing as a norm.

Console.WriteLine($"Root Mean Squared Error: {metrics.RootMeanSquaredError:F2}");
Console.WriteLine($"RSquared: {metrics.RSquared:F2}");
Console.WriteLine("Mean Absolute Error: " + metrics.MeanAbsoluteError +
":F2");

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.

Is this correct? I thought F2 tells the prcision.

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'm not sure either. I tried reformatting it like another example Zeeshan helped us with that used a similar format. Do you know what the right notation is?

":F2");

Console.WriteLine("Mean Squared Error: " + metrics.MeanSquaredError +
":F2");

@wschinwschinJul 1, 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.

Maybe merging the two lines to

Console.WriteLine("Mean Squared Error: " + metrics.MeanSquaredError);

could be a reasonable setting.

// its desired value would be set 0.15. In other words, this parameter determines
// the value of all missing matrix elements.
MatrixColumnIndexColumnName = nameof(
MatrixElement.MatrixColumnIndex),

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.

Line 46 and Lines 47-53 should be at the same indent level.

@sayanshaw24sayanshaw24Jul 2, 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.

Zeeshan asked us to indent after the first break, so we've been doing that for all the files. Maybe we can discuss this along with Zeeshan in the scrum tomorrow?

@codemzscodemzs left a comment

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.

:shipit:

@sayanshaw24
sayanshaw24 merged commit 01b3ec7 into dotnet:masterJul 2, 2019
var predictions = mlContext.Data.CreateEnumerable<MatrixElement>(transformedData, reuseRowObject: false).Take(5).ToList();
var predictions = mlContext.Data
.CreateEnumerable<MatrixElement>(transformedData,
reuseRowObject: false).Take(5).ToList();

@sharwellsharwellJul 2, 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.

📝 This layout is very difficult to read. The following would be preferable:

varpredictions=mlContext.Data.CreateEnumerable<MatrixElement>(transformedData,reuseRowObject:false).Take(5).ToList();

nameof(MatrixElement.MatrixRowIndex), 10, 0.2, 1);
var pipeline = mlContext.Recommendation().Trainers.
MatrixFactorization(nameof(MatrixElement.Value),
nameof(MatrixElement.MatrixColumnIndex),

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.

📝 This is incorrectly indented

for (uint j = 0; j < MatrixRowCount; ++j)
dataMatrix.Add(new MatrixElement() { MatrixColumnIndex = i, MatrixRowIndex = j, Value = (i + j) % 5 });
dataMatrix.Add(new MatrixElement() { MatrixColumnIndex = i,
MatrixRowIndex = j, Value = (i + j) % 5 });

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.

📝 Incorrect line wrapping and indentation

@sharwellsharwell 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.

Requesting changes. The wrapping applied in this pull request produces worse readability than whatever tool was automatically wrapping past the 85 character limit.

// the value of all missing matrix elements.
MatrixColumnIndexColumnName = nameof(
MatrixElement.MatrixColumnIndex),
MatrixRowIndexColumnName = nameof(MatrixElement.MatrixRowIndex),

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.

📝 This is incorrectly indented

// Apply the trained model to the test set. Notice that training is a
// partial
var prediction = model.Transform(mlContext.Data.LoadFromEnumerable(
testData));

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.

📝 Incorrect wrapping (prefer to wrap the outer argument when it will fit on the same number of lines)

// Specify IDataView colum which stores matrix row indexes.
// Specify IDataView colum which stores matrix column indexes.
MatrixColumnIndexColumnName = nameof(MatrixElement.MatrixColumnIndex
),

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.

📝 Incorrect wrapping (should be wrapped before nameof)

Dmitry-A pushed a commit to Dmitry-A/machinelearning that referenced this pull request Jul 24, 2019
* Reformatted Recommendation samples
* Fix for tab spacing errors
* Removed precision for reformatting in Console.WriteLine and fixed KeyType error
@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.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@sayanshaw24@codemzs@sharwell@natke@wschin
, '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

Reformatted Recommendation samples to width 85 - #3941

Merged
sayanshaw24 merged 3 commits into
dotnet:masterfrom
sayanshaw24:recommendationReformat
Jul 2, 2019
Merged

Reformatted Recommendation samples to width 85#3941
sayanshaw24 merged 3 commits into
dotnet:masterfrom
sayanshaw24:recommendationReformat

Conversation

@sayanshaw24

Copy link
Copy Markdown
Contributor

Guidelines followed:

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

dataMatrix.Add(new MatrixElement() { MatrixColumnIndex = i, MatrixRowIndex = j, Value = (i + j) % 5 });
dataMatrix.Add(new MatrixElement() { MatrixColumnIndex = i,
MatrixRowIndex = j, Value = (i + j) % 5 });

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.

Empty line. Is it produced by an auto-formatting tool?

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.

Actually, Zeeshan asked us to do that, so that's what we've been doing as a norm.

Console.WriteLine($"Root Mean Squared Error: {metrics.RootMeanSquaredError:F2}");
Console.WriteLine($"RSquared: {metrics.RSquared:F2}");
Console.WriteLine("Mean Absolute Error: " + metrics.MeanAbsoluteError +
":F2");

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.

Is this correct? I thought F2 tells the prcision.

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'm not sure either. I tried reformatting it like another example Zeeshan helped us with that used a similar format. Do you know what the right notation is?

":F2");

Console.WriteLine("Mean Squared Error: " + metrics.MeanSquaredError +
":F2");

@wschinwschinJul 1, 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.

Maybe merging the two lines to

Console.WriteLine("Mean Squared Error: " + metrics.MeanSquaredError);

could be a reasonable setting.

// its desired value would be set 0.15. In other words, this parameter determines
// the value of all missing matrix elements.
MatrixColumnIndexColumnName = nameof(
MatrixElement.MatrixColumnIndex),

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.

Line 46 and Lines 47-53 should be at the same indent level.

@sayanshaw24sayanshaw24Jul 2, 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.

Zeeshan asked us to indent after the first break, so we've been doing that for all the files. Maybe we can discuss this along with Zeeshan in the scrum tomorrow?

@codemzscodemzs left a comment

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.

:shipit:

@sayanshaw24
sayanshaw24 merged commit 01b3ec7 into dotnet:masterJul 2, 2019
var predictions = mlContext.Data.CreateEnumerable<MatrixElement>(transformedData, reuseRowObject: false).Take(5).ToList();
var predictions = mlContext.Data
.CreateEnumerable<MatrixElement>(transformedData,
reuseRowObject: false).Take(5).ToList();

@sharwellsharwellJul 2, 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.

📝 This layout is very difficult to read. The following would be preferable:

varpredictions=mlContext.Data.CreateEnumerable<MatrixElement>(transformedData,reuseRowObject:false).Take(5).ToList();

nameof(MatrixElement.MatrixRowIndex), 10, 0.2, 1);
var pipeline = mlContext.Recommendation().Trainers.
MatrixFactorization(nameof(MatrixElement.Value),
nameof(MatrixElement.MatrixColumnIndex),

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.

📝 This is incorrectly indented

for (uint j = 0; j < MatrixRowCount; ++j)
dataMatrix.Add(new MatrixElement() { MatrixColumnIndex = i, MatrixRowIndex = j, Value = (i + j) % 5 });
dataMatrix.Add(new MatrixElement() { MatrixColumnIndex = i,
MatrixRowIndex = j, Value = (i + j) % 5 });

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.

📝 Incorrect line wrapping and indentation

@sharwellsharwell 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.

Requesting changes. The wrapping applied in this pull request produces worse readability than whatever tool was automatically wrapping past the 85 character limit.

// the value of all missing matrix elements.
MatrixColumnIndexColumnName = nameof(
MatrixElement.MatrixColumnIndex),
MatrixRowIndexColumnName = nameof(MatrixElement.MatrixRowIndex),

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.

📝 This is incorrectly indented

// Apply the trained model to the test set. Notice that training is a
// partial
var prediction = model.Transform(mlContext.Data.LoadFromEnumerable(
testData));

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.

📝 Incorrect wrapping (prefer to wrap the outer argument when it will fit on the same number of lines)

// Specify IDataView colum which stores matrix row indexes.
// Specify IDataView colum which stores matrix column indexes.
MatrixColumnIndexColumnName = nameof(MatrixElement.MatrixColumnIndex
),

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.

📝 Incorrect wrapping (should be wrapped before nameof)

Dmitry-A pushed a commit to Dmitry-A/machinelearning that referenced this pull request Jul 24, 2019
* Reformatted Recommendation samples
* Fix for tab spacing errors
* Removed precision for reformatting in Console.WriteLine and fixed KeyType error
@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.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@sayanshaw24@codemzs@sharwell@natke@wschin
, '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

Reformatted Recommendation samples to width 85 - #3941

Merged
sayanshaw24 merged 3 commits into
dotnet:masterfrom
sayanshaw24:recommendationReformat
Jul 2, 2019
Merged

Reformatted Recommendation samples to width 85#3941
sayanshaw24 merged 3 commits into
dotnet:masterfrom
sayanshaw24:recommendationReformat

Conversation

@sayanshaw24

Copy link
Copy Markdown
Contributor

Guidelines followed:

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

dataMatrix.Add(new MatrixElement() { MatrixColumnIndex = i, MatrixRowIndex = j, Value = (i + j) % 5 });
dataMatrix.Add(new MatrixElement() { MatrixColumnIndex = i,
MatrixRowIndex = j, Value = (i + j) % 5 });

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.

Empty line. Is it produced by an auto-formatting tool?

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.

Actually, Zeeshan asked us to do that, so that's what we've been doing as a norm.

Console.WriteLine($"Root Mean Squared Error: {metrics.RootMeanSquaredError:F2}");
Console.WriteLine($"RSquared: {metrics.RSquared:F2}");
Console.WriteLine("Mean Absolute Error: " + metrics.MeanAbsoluteError +
":F2");

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.

Is this correct? I thought F2 tells the prcision.

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'm not sure either. I tried reformatting it like another example Zeeshan helped us with that used a similar format. Do you know what the right notation is?

":F2");

Console.WriteLine("Mean Squared Error: " + metrics.MeanSquaredError +
":F2");

@wschinwschinJul 1, 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.

Maybe merging the two lines to

Console.WriteLine("Mean Squared Error: " + metrics.MeanSquaredError);

could be a reasonable setting.

// its desired value would be set 0.15. In other words, this parameter determines
// the value of all missing matrix elements.
MatrixColumnIndexColumnName = nameof(
MatrixElement.MatrixColumnIndex),

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.

Line 46 and Lines 47-53 should be at the same indent level.

@sayanshaw24sayanshaw24Jul 2, 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.

Zeeshan asked us to indent after the first break, so we've been doing that for all the files. Maybe we can discuss this along with Zeeshan in the scrum tomorrow?

@codemzscodemzs left a comment

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.

:shipit:

@sayanshaw24
sayanshaw24 merged commit 01b3ec7 into dotnet:masterJul 2, 2019
var predictions = mlContext.Data.CreateEnumerable<MatrixElement>(transformedData, reuseRowObject: false).Take(5).ToList();
var predictions = mlContext.Data
.CreateEnumerable<MatrixElement>(transformedData,
reuseRowObject: false).Take(5).ToList();

@sharwellsharwellJul 2, 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.

📝 This layout is very difficult to read. The following would be preferable:

varpredictions=mlContext.Data.CreateEnumerable<MatrixElement>(transformedData,reuseRowObject:false).Take(5).ToList();

nameof(MatrixElement.MatrixRowIndex), 10, 0.2, 1);
var pipeline = mlContext.Recommendation().Trainers.
MatrixFactorization(nameof(MatrixElement.Value),
nameof(MatrixElement.MatrixColumnIndex),

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.

📝 This is incorrectly indented

for (uint j = 0; j < MatrixRowCount; ++j)
dataMatrix.Add(new MatrixElement() { MatrixColumnIndex = i, MatrixRowIndex = j, Value = (i + j) % 5 });
dataMatrix.Add(new MatrixElement() { MatrixColumnIndex = i,
MatrixRowIndex = j, Value = (i + j) % 5 });

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.

📝 Incorrect line wrapping and indentation

@sharwellsharwell 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.

Requesting changes. The wrapping applied in this pull request produces worse readability than whatever tool was automatically wrapping past the 85 character limit.

// the value of all missing matrix elements.
MatrixColumnIndexColumnName = nameof(
MatrixElement.MatrixColumnIndex),
MatrixRowIndexColumnName = nameof(MatrixElement.MatrixRowIndex),

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.

📝 This is incorrectly indented

// Apply the trained model to the test set. Notice that training is a
// partial
var prediction = model.Transform(mlContext.Data.LoadFromEnumerable(
testData));

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.

📝 Incorrect wrapping (prefer to wrap the outer argument when it will fit on the same number of lines)

// Specify IDataView colum which stores matrix row indexes.
// Specify IDataView colum which stores matrix column indexes.
MatrixColumnIndexColumnName = nameof(MatrixElement.MatrixColumnIndex
),

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.

📝 Incorrect wrapping (should be wrapped before nameof)

Dmitry-A pushed a commit to Dmitry-A/machinelearning that referenced this pull request Jul 24, 2019
* Reformatted Recommendation samples
* Fix for tab spacing errors
* Removed precision for reformatting in Console.WriteLine and fixed KeyType error
@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.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@sayanshaw24@codemzs@sharwell@natke@wschin
, '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

Reformatted Recommendation samples to width 85 - #3941

Merged
sayanshaw24 merged 3 commits into
dotnet:masterfrom
sayanshaw24:recommendationReformat
Jul 2, 2019
Merged

Reformatted Recommendation samples to width 85#3941
sayanshaw24 merged 3 commits into
dotnet:masterfrom
sayanshaw24:recommendationReformat

Conversation

@sayanshaw24

Copy link
Copy Markdown
Contributor

Guidelines followed:

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

dataMatrix.Add(new MatrixElement() { MatrixColumnIndex = i, MatrixRowIndex = j, Value = (i + j) % 5 });
dataMatrix.Add(new MatrixElement() { MatrixColumnIndex = i,
MatrixRowIndex = j, Value = (i + j) % 5 });

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.

Empty line. Is it produced by an auto-formatting tool?

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.

Actually, Zeeshan asked us to do that, so that's what we've been doing as a norm.

Console.WriteLine($"Root Mean Squared Error: {metrics.RootMeanSquaredError:F2}");
Console.WriteLine($"RSquared: {metrics.RSquared:F2}");
Console.WriteLine("Mean Absolute Error: " + metrics.MeanAbsoluteError +
":F2");

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.

Is this correct? I thought F2 tells the prcision.

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'm not sure either. I tried reformatting it like another example Zeeshan helped us with that used a similar format. Do you know what the right notation is?

":F2");

Console.WriteLine("Mean Squared Error: " + metrics.MeanSquaredError +
":F2");

@wschinwschinJul 1, 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.

Maybe merging the two lines to

Console.WriteLine("Mean Squared Error: " + metrics.MeanSquaredError);

could be a reasonable setting.

// its desired value would be set 0.15. In other words, this parameter determines
// the value of all missing matrix elements.
MatrixColumnIndexColumnName = nameof(
MatrixElement.MatrixColumnIndex),

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.

Line 46 and Lines 47-53 should be at the same indent level.

@sayanshaw24sayanshaw24Jul 2, 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.

Zeeshan asked us to indent after the first break, so we've been doing that for all the files. Maybe we can discuss this along with Zeeshan in the scrum tomorrow?

@codemzscodemzs left a comment

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.

:shipit:

@sayanshaw24
sayanshaw24 merged commit 01b3ec7 into dotnet:masterJul 2, 2019
var predictions = mlContext.Data.CreateEnumerable<MatrixElement>(transformedData, reuseRowObject: false).Take(5).ToList();
var predictions = mlContext.Data
.CreateEnumerable<MatrixElement>(transformedData,
reuseRowObject: false).Take(5).ToList();

@sharwellsharwellJul 2, 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.

📝 This layout is very difficult to read. The following would be preferable:

varpredictions=mlContext.Data.CreateEnumerable<MatrixElement>(transformedData,reuseRowObject:false).Take(5).ToList();

nameof(MatrixElement.MatrixRowIndex), 10, 0.2, 1);
var pipeline = mlContext.Recommendation().Trainers.
MatrixFactorization(nameof(MatrixElement.Value),
nameof(MatrixElement.MatrixColumnIndex),

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.

📝 This is incorrectly indented

for (uint j = 0; j < MatrixRowCount; ++j)
dataMatrix.Add(new MatrixElement() { MatrixColumnIndex = i, MatrixRowIndex = j, Value = (i + j) % 5 });
dataMatrix.Add(new MatrixElement() { MatrixColumnIndex = i,
MatrixRowIndex = j, Value = (i + j) % 5 });

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.

📝 Incorrect line wrapping and indentation

@sharwellsharwell 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.

Requesting changes. The wrapping applied in this pull request produces worse readability than whatever tool was automatically wrapping past the 85 character limit.

// the value of all missing matrix elements.
MatrixColumnIndexColumnName = nameof(
MatrixElement.MatrixColumnIndex),
MatrixRowIndexColumnName = nameof(MatrixElement.MatrixRowIndex),

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.

📝 This is incorrectly indented

// Apply the trained model to the test set. Notice that training is a
// partial
var prediction = model.Transform(mlContext.Data.LoadFromEnumerable(
testData));

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.

📝 Incorrect wrapping (prefer to wrap the outer argument when it will fit on the same number of lines)

// Specify IDataView colum which stores matrix row indexes.
// Specify IDataView colum which stores matrix column indexes.
MatrixColumnIndexColumnName = nameof(MatrixElement.MatrixColumnIndex
),

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.

📝 Incorrect wrapping (should be wrapped before nameof)

Dmitry-A pushed a commit to Dmitry-A/machinelearning that referenced this pull request Jul 24, 2019
* Reformatted Recommendation samples
* Fix for tab spacing errors
* Removed precision for reformatting in Console.WriteLine and fixed KeyType error
@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.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@sayanshaw24@codemzs@sharwell@natke@wschin
, '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

Reformatted Recommendation samples to width 85 - #3941

Merged
sayanshaw24 merged 3 commits into
dotnet:masterfrom
sayanshaw24:recommendationReformat
Jul 2, 2019
Merged

Reformatted Recommendation samples to width 85#3941
sayanshaw24 merged 3 commits into
dotnet:masterfrom
sayanshaw24:recommendationReformat

Conversation

@sayanshaw24

Copy link
Copy Markdown
Contributor

Guidelines followed:

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

dataMatrix.Add(new MatrixElement() { MatrixColumnIndex = i, MatrixRowIndex = j, Value = (i + j) % 5 });
dataMatrix.Add(new MatrixElement() { MatrixColumnIndex = i,
MatrixRowIndex = j, Value = (i + j) % 5 });

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.

Empty line. Is it produced by an auto-formatting tool?

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.

Actually, Zeeshan asked us to do that, so that's what we've been doing as a norm.

Console.WriteLine($"Root Mean Squared Error: {metrics.RootMeanSquaredError:F2}");
Console.WriteLine($"RSquared: {metrics.RSquared:F2}");
Console.WriteLine("Mean Absolute Error: " + metrics.MeanAbsoluteError +
":F2");

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.

Is this correct? I thought F2 tells the prcision.

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'm not sure either. I tried reformatting it like another example Zeeshan helped us with that used a similar format. Do you know what the right notation is?

":F2");

Console.WriteLine("Mean Squared Error: " + metrics.MeanSquaredError +
":F2");

@wschinwschinJul 1, 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.

Maybe merging the two lines to

Console.WriteLine("Mean Squared Error: " + metrics.MeanSquaredError);

could be a reasonable setting.

// its desired value would be set 0.15. In other words, this parameter determines
// the value of all missing matrix elements.
MatrixColumnIndexColumnName = nameof(
MatrixElement.MatrixColumnIndex),

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.

Line 46 and Lines 47-53 should be at the same indent level.

@sayanshaw24sayanshaw24Jul 2, 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.

Zeeshan asked us to indent after the first break, so we've been doing that for all the files. Maybe we can discuss this along with Zeeshan in the scrum tomorrow?

@codemzscodemzs left a comment

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.

:shipit:

@sayanshaw24
sayanshaw24 merged commit 01b3ec7 into dotnet:masterJul 2, 2019
var predictions = mlContext.Data.CreateEnumerable<MatrixElement>(transformedData, reuseRowObject: false).Take(5).ToList();
var predictions = mlContext.Data
.CreateEnumerable<MatrixElement>(transformedData,
reuseRowObject: false).Take(5).ToList();

@sharwellsharwellJul 2, 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.

📝 This layout is very difficult to read. The following would be preferable:

varpredictions=mlContext.Data.CreateEnumerable<MatrixElement>(transformedData,reuseRowObject:false).Take(5).ToList();

nameof(MatrixElement.MatrixRowIndex), 10, 0.2, 1);
var pipeline = mlContext.Recommendation().Trainers.
MatrixFactorization(nameof(MatrixElement.Value),
nameof(MatrixElement.MatrixColumnIndex),

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.

📝 This is incorrectly indented

for (uint j = 0; j < MatrixRowCount; ++j)
dataMatrix.Add(new MatrixElement() { MatrixColumnIndex = i, MatrixRowIndex = j, Value = (i + j) % 5 });
dataMatrix.Add(new MatrixElement() { MatrixColumnIndex = i,
MatrixRowIndex = j, Value = (i + j) % 5 });

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.

📝 Incorrect line wrapping and indentation

@sharwellsharwell 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.

Requesting changes. The wrapping applied in this pull request produces worse readability than whatever tool was automatically wrapping past the 85 character limit.

// the value of all missing matrix elements.
MatrixColumnIndexColumnName = nameof(
MatrixElement.MatrixColumnIndex),
MatrixRowIndexColumnName = nameof(MatrixElement.MatrixRowIndex),

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.

📝 This is incorrectly indented

// Apply the trained model to the test set. Notice that training is a
// partial
var prediction = model.Transform(mlContext.Data.LoadFromEnumerable(
testData));

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.

📝 Incorrect wrapping (prefer to wrap the outer argument when it will fit on the same number of lines)

// Specify IDataView colum which stores matrix row indexes.
// Specify IDataView colum which stores matrix column indexes.
MatrixColumnIndexColumnName = nameof(MatrixElement.MatrixColumnIndex
),

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.

📝 Incorrect wrapping (should be wrapped before nameof)

Dmitry-A pushed a commit to Dmitry-A/machinelearning that referenced this pull request Jul 24, 2019
* Reformatted Recommendation samples
* Fix for tab spacing errors
* Removed precision for reformatting in Console.WriteLine and fixed KeyType error
@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.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@sayanshaw24@codemzs@sharwell@natke@wschin
, '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

Reformatted Recommendation samples to width 85 - #3941

Merged
sayanshaw24 merged 3 commits into
dotnet:masterfrom
sayanshaw24:recommendationReformat
Jul 2, 2019
Merged

Reformatted Recommendation samples to width 85#3941
sayanshaw24 merged 3 commits into
dotnet:masterfrom
sayanshaw24:recommendationReformat

Conversation

@sayanshaw24

Copy link
Copy Markdown
Contributor

Guidelines followed:

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

dataMatrix.Add(new MatrixElement() { MatrixColumnIndex = i, MatrixRowIndex = j, Value = (i + j) % 5 });
dataMatrix.Add(new MatrixElement() { MatrixColumnIndex = i,
MatrixRowIndex = j, Value = (i + j) % 5 });

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.

Empty line. Is it produced by an auto-formatting tool?

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.

Actually, Zeeshan asked us to do that, so that's what we've been doing as a norm.

Console.WriteLine($"Root Mean Squared Error: {metrics.RootMeanSquaredError:F2}");
Console.WriteLine($"RSquared: {metrics.RSquared:F2}");
Console.WriteLine("Mean Absolute Error: " + metrics.MeanAbsoluteError +
":F2");

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.

Is this correct? I thought F2 tells the prcision.

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'm not sure either. I tried reformatting it like another example Zeeshan helped us with that used a similar format. Do you know what the right notation is?

":F2");

Console.WriteLine("Mean Squared Error: " + metrics.MeanSquaredError +
":F2");

@wschinwschinJul 1, 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.

Maybe merging the two lines to

Console.WriteLine("Mean Squared Error: " + metrics.MeanSquaredError);

could be a reasonable setting.

// its desired value would be set 0.15. In other words, this parameter determines
// the value of all missing matrix elements.
MatrixColumnIndexColumnName = nameof(
MatrixElement.MatrixColumnIndex),

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.

Line 46 and Lines 47-53 should be at the same indent level.

@sayanshaw24sayanshaw24Jul 2, 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.

Zeeshan asked us to indent after the first break, so we've been doing that for all the files. Maybe we can discuss this along with Zeeshan in the scrum tomorrow?

@codemzscodemzs left a comment

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.

:shipit:

@sayanshaw24
sayanshaw24 merged commit 01b3ec7 into dotnet:masterJul 2, 2019
var predictions = mlContext.Data.CreateEnumerable<MatrixElement>(transformedData, reuseRowObject: false).Take(5).ToList();
var predictions = mlContext.Data
.CreateEnumerable<MatrixElement>(transformedData,
reuseRowObject: false).Take(5).ToList();

@sharwellsharwellJul 2, 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.

📝 This layout is very difficult to read. The following would be preferable:

varpredictions=mlContext.Data.CreateEnumerable<MatrixElement>(transformedData,reuseRowObject:false).Take(5).ToList();

nameof(MatrixElement.MatrixRowIndex), 10, 0.2, 1);
var pipeline = mlContext.Recommendation().Trainers.
MatrixFactorization(nameof(MatrixElement.Value),
nameof(MatrixElement.MatrixColumnIndex),

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.

📝 This is incorrectly indented

for (uint j = 0; j < MatrixRowCount; ++j)
dataMatrix.Add(new MatrixElement() { MatrixColumnIndex = i, MatrixRowIndex = j, Value = (i + j) % 5 });
dataMatrix.Add(new MatrixElement() { MatrixColumnIndex = i,
MatrixRowIndex = j, Value = (i + j) % 5 });

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.

📝 Incorrect line wrapping and indentation

@sharwellsharwell 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.

Requesting changes. The wrapping applied in this pull request produces worse readability than whatever tool was automatically wrapping past the 85 character limit.

// the value of all missing matrix elements.
MatrixColumnIndexColumnName = nameof(
MatrixElement.MatrixColumnIndex),
MatrixRowIndexColumnName = nameof(MatrixElement.MatrixRowIndex),

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.

📝 This is incorrectly indented

// Apply the trained model to the test set. Notice that training is a
// partial
var prediction = model.Transform(mlContext.Data.LoadFromEnumerable(
testData));

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.

📝 Incorrect wrapping (prefer to wrap the outer argument when it will fit on the same number of lines)

// Specify IDataView colum which stores matrix row indexes.
// Specify IDataView colum which stores matrix column indexes.
MatrixColumnIndexColumnName = nameof(MatrixElement.MatrixColumnIndex
),

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.

📝 Incorrect wrapping (should be wrapped before nameof)

Dmitry-A pushed a commit to Dmitry-A/machinelearning that referenced this pull request Jul 24, 2019
* Reformatted Recommendation samples
* Fix for tab spacing errors
* Removed precision for reformatting in Console.WriteLine and fixed KeyType error
@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.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@sayanshaw24@codemzs@sharwell@natke@wschin
, '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

Reformatted Recommendation samples to width 85 - #3941

Merged
sayanshaw24 merged 3 commits into
dotnet:masterfrom
sayanshaw24:recommendationReformat
Jul 2, 2019
Merged

Reformatted Recommendation samples to width 85#3941
sayanshaw24 merged 3 commits into
dotnet:masterfrom
sayanshaw24:recommendationReformat

Conversation

@sayanshaw24

Copy link
Copy Markdown
Contributor

Guidelines followed:

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

dataMatrix.Add(new MatrixElement() { MatrixColumnIndex = i, MatrixRowIndex = j, Value = (i + j) % 5 });
dataMatrix.Add(new MatrixElement() { MatrixColumnIndex = i,
MatrixRowIndex = j, Value = (i + j) % 5 });

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.

Empty line. Is it produced by an auto-formatting tool?

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.

Actually, Zeeshan asked us to do that, so that's what we've been doing as a norm.

Console.WriteLine($"Root Mean Squared Error: {metrics.RootMeanSquaredError:F2}");
Console.WriteLine($"RSquared: {metrics.RSquared:F2}");
Console.WriteLine("Mean Absolute Error: " + metrics.MeanAbsoluteError +
":F2");

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.

Is this correct? I thought F2 tells the prcision.

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'm not sure either. I tried reformatting it like another example Zeeshan helped us with that used a similar format. Do you know what the right notation is?

":F2");

Console.WriteLine("Mean Squared Error: " + metrics.MeanSquaredError +
":F2");

@wschinwschinJul 1, 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.

Maybe merging the two lines to

Console.WriteLine("Mean Squared Error: " + metrics.MeanSquaredError);

could be a reasonable setting.

// its desired value would be set 0.15. In other words, this parameter determines
// the value of all missing matrix elements.
MatrixColumnIndexColumnName = nameof(
MatrixElement.MatrixColumnIndex),

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.

Line 46 and Lines 47-53 should be at the same indent level.

@sayanshaw24sayanshaw24Jul 2, 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.

Zeeshan asked us to indent after the first break, so we've been doing that for all the files. Maybe we can discuss this along with Zeeshan in the scrum tomorrow?

@codemzscodemzs left a comment

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.

:shipit:

@sayanshaw24
sayanshaw24 merged commit 01b3ec7 into dotnet:masterJul 2, 2019
var predictions = mlContext.Data.CreateEnumerable<MatrixElement>(transformedData, reuseRowObject: false).Take(5).ToList();
var predictions = mlContext.Data
.CreateEnumerable<MatrixElement>(transformedData,
reuseRowObject: false).Take(5).ToList();

@sharwellsharwellJul 2, 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.

📝 This layout is very difficult to read. The following would be preferable:

varpredictions=mlContext.Data.CreateEnumerable<MatrixElement>(transformedData,reuseRowObject:false).Take(5).ToList();

nameof(MatrixElement.MatrixRowIndex), 10, 0.2, 1);
var pipeline = mlContext.Recommendation().Trainers.
MatrixFactorization(nameof(MatrixElement.Value),
nameof(MatrixElement.MatrixColumnIndex),

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.

📝 This is incorrectly indented

for (uint j = 0; j < MatrixRowCount; ++j)
dataMatrix.Add(new MatrixElement() { MatrixColumnIndex = i, MatrixRowIndex = j, Value = (i + j) % 5 });
dataMatrix.Add(new MatrixElement() { MatrixColumnIndex = i,
MatrixRowIndex = j, Value = (i + j) % 5 });

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.

📝 Incorrect line wrapping and indentation

@sharwellsharwell 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.

Requesting changes. The wrapping applied in this pull request produces worse readability than whatever tool was automatically wrapping past the 85 character limit.

// the value of all missing matrix elements.
MatrixColumnIndexColumnName = nameof(
MatrixElement.MatrixColumnIndex),
MatrixRowIndexColumnName = nameof(MatrixElement.MatrixRowIndex),

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.

📝 This is incorrectly indented

// Apply the trained model to the test set. Notice that training is a
// partial
var prediction = model.Transform(mlContext.Data.LoadFromEnumerable(
testData));

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.

📝 Incorrect wrapping (prefer to wrap the outer argument when it will fit on the same number of lines)

// Specify IDataView colum which stores matrix row indexes.
// Specify IDataView colum which stores matrix column indexes.
MatrixColumnIndexColumnName = nameof(MatrixElement.MatrixColumnIndex
),

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.

📝 Incorrect wrapping (should be wrapped before nameof)

Dmitry-A pushed a commit to Dmitry-A/machinelearning that referenced this pull request Jul 24, 2019
* Reformatted Recommendation samples
* Fix for tab spacing errors
* Removed precision for reformatting in Console.WriteLine and fixed KeyType error
@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.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@sayanshaw24@codemzs@sharwell@natke@wschin