Skip to content

C# samples should probably use C# and not Python naming conventions #2155

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

Closed
TomFinley opened this issue Jan 15, 2019 · 6 comments
Closed

C# samples should probably use C# and not Python naming conventions #2155

TomFinley opened this issue Jan 15, 2019 · 6 comments
Labels
documentation Related to documentation of ML.NET good first issue Good for newcomers up-for-grabs A good issue to fix if you are trying to contribute to the project

Comments

@TomFinley
Copy link
Contributor

TomFinley commented Jan 15, 2019

I was reading a PR when suddenly I saw something a bit odd.

var customized_pipeline = new WordTokenizingEstimator(ml, "Review")
.Append(new ValueToKeyMappingEstimator(ml, "Review", customizedColumnName, maxNumTerms: 10, sort: ValueToKeyMappingTransformer.SortOrder.Value));
// The transformed data.
var transformedData_default = default_pipeline.Fit(trainData).Transform(trainData);
var transformedData_customized = customized_pipeline.Fit(trainData).Transform(trainData);

Note the underscores in the middle of a variable name. While in private we might elect to bend the rules a little bit, in our public API and also probably our samples, we probably ought to write them with consideration of standard practices. This does not seem to be a justifiable or explainable diversion from the recommended practice, and I'm guessing it was added by mistake.

Each individual renaming is probably not huge, but might span several files, since the example listed above is just one of many examples I see in our samples project. So not a "hard" change, except possibly in scale?

@TomFinley TomFinley added good first issue Good for newcomers up-for-grabs A good issue to fix if you are trying to contribute to the project documentation Related to documentation of ML.NET labels Jan 15, 2019
@ghost
Copy link

ghost commented Jan 18, 2019

This would be a good first issue for me, can I take this?

@artidoro
Copy link
Contributor

Sure! We would welcome your contribution and that looks like a good first issue.

@ghost
Copy link

ghost commented Jan 20, 2019

Thanks, you can assign this to me, and ill begin working on it

sfilipi added a commit to sfilipi/machinelearning-1 that referenced this issue Jan 30, 2019
sfilipi added a commit that referenced this issue Jan 30, 2019
* fixing a link in the cookbook

* making sure issue #2155 has no issues with the cookbooks

* nit: title clarification
@sfilipi
Copy link
Member

sfilipi commented Feb 5, 2019

This is completed with PR #2303

@sfilipi sfilipi closed this as completed Feb 5, 2019
@sfilipi
Copy link
Member

sfilipi commented Feb 5, 2019

reopening, just noticed that those PRs, didn't get merge..

@sfilipi sfilipi reopened this Feb 5, 2019
@shmoradims
Copy link

Samples were cleaned up as part of 1.0 API reference documentation. Please re-open with any wring naming instance you find.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Related to documentation of ML.NET good first issue Good for newcomers up-for-grabs A good issue to fix if you are trying to contribute to the project
Projects
None yet
Development

No branches or pull requests

4 participants