Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed python naming conventions from C# samples #2203

Closed
wants to merge 3 commits into from
Closed

Removed python naming conventions from C# samples #2203

wants to merge 3 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jan 22, 2019

Fixes #2155

Removed the python naming conventions in docs/samples/Microsoft.ML.Samples

@dnfclas
Copy link

dnfclas commented Jan 22, 2019

CLA assistant check
All CLA requirements met.

Copy link
Contributor

@rogancarr rogancarr left a comment

Choose a reason for hiding this comment

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

Hi @daniel-loudon, thanks for sending in the PR! Overall, this is great. I've made a couple comments here and there. When you resubmit changes, make sure to fill out the description for the PR — the instructions are in the description box.

var transformedData_default = default_pipeline.Fit(trainData).Transform(trainData);
var transformedData_customized = customized_pipeline.Fit(trainData).Transform(trainData);
var transformedDataDefault = defaultPipeline.Fit(trainData).Transform(trainData);
var transformedDatacustomized = customizedPipeline.Fit(trainData).Transform(trainData);
Copy link
Contributor

Choose a reason for hiding this comment

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

transformedDataCustomized

@@ -10,23 +10,23 @@ public class MatrixFactorizationExample
// The variable _synthesizedMatrixFirstRowIndex indicates the integer that would be mapped to the first row index. If user data uses
// 0-based indices for rows, _synthesizedMatrixFirstRowIndex can be set to 0. Similarly, for 1-based indices, _synthesizedMatrixFirstRowIndex
// could be 1.
const int _synthesizedMatrixFirstColumnIndex = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep these with the underscore (_), but mark them private.

Copy link
Author

Choose a reason for hiding this comment

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

Ok I didn't think any names should have an underscore regardless of access

Copy link
Contributor

Choose a reason for hiding this comment

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

The convention we follow is that private variables start with underscores and public variables start with caps.

@@ -33,8 +33,8 @@ public static void NgramTransform()
var twoCharsPipeline = charsPipeline.Append(ngramTwpPipeline);

// The transformed data for pipelines.
var transformedData_onechars = oneCharsPipeline.Fit(trainData).Transform(trainData);
var transformedData_twochars = twoCharsPipeline.Fit(trainData).Transform(trainData);
var transformedDataOnechars = oneCharsPipeline.Fit(trainData).Transform(trainData);
Copy link
Contributor

Choose a reason for hiding this comment

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

OneChars

Copy link
Contributor

@rogancarr rogancarr left a comment

Choose a reason for hiding this comment

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

Looks good; just one comment and you need to fix the merge issues and you are good to go!

var twoCharsPipeline = charsPipeline.Append(ngramTwpPipeline);

// The transformed data for pipelines.
var transformedData_onechars = oneCharsPipeline.Fit(trainData).Transform(trainData);
var transformedData_twochars = twoCharsPipeline.Fit(trainData).Transform(trainData);
var transformedDataOnechar = oneCharPipeline.Fit(trainData).Transform(trainData);
Copy link
Contributor

Choose a reason for hiding this comment

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

OneChar

@Ivanidzo4ka
Copy link
Contributor

@daniel-loudon Do you have plans to update this PR to latest master and resolve conflicts?

@Ivanidzo4ka
Copy link
Contributor

@daniel-loudon Thank you for updating this PR.
It just looks like something went terribly wrong.

Files changed 563

That doesn't look like proper merge with latest master.

@ghost
Copy link
Author

ghost commented Jan 29, 2019

@daniel-loudon Thank you for updating this PR.
It just looks like something went terribly wrong.

Files changed 563

That doesn't look like proper merge with latest master.

Yeah something didnt go right with my upstream, shall I try again?

@ghost ghost closed this Jan 29, 2019
@ghost
Copy link
Author

ghost commented Jan 29, 2019

I will redo this in the morning and open a new PR, I couldn't fetch any commits from this respository. Is that cool? I only learn from my mistakes.

@Ivanidzo4ka
Copy link
Contributor

@daniel-loudon No worries.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants