Skip to content

Reformatting TensorFlow and AnomalyDetection samples to width 85 #3922

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Jun 29, 2019
Merged

Reformatting TensorFlow and AnomalyDetection samples to width 85 #3922

merged 9 commits into from
Jun 29, 2019

Conversation

sayanshaw24
Copy link
Contributor

@sayanshaw24 sayanshaw24 commented Jun 27, 2019 •
edited
Loading

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 #3478

@dnfclas
Copy link

dnfclas commented Jun 27, 2019 •
edited
Loading

CLA assistant check
All CLA requirements met.

@sayanshaw24 sayanshaw24 requested a review from natke June 27, 2019 17:28
wschin
wschin approved these changes Jun 27, 2019
View reviewed changes
Copy link
Member

@wschin wschin left a comment

Choose a reason for hiding this comment

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

Just left some minor comments. Overall LGTM.

@sayanshaw24 sayanshaw24 changed the title Reformatting TensorFlow samples to width 85 Reformatting TensorFlow and AnomalyDetection samples to width 85 Jun 27, 2019
natke
natke reviewed Jun 28, 2019
View reviewed changes
Copy link
Contributor

@natke natke left a comment

Choose a reason for hiding this comment

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

A couple of suggestions

docs/samples/Microsoft.ML.Samples/Dynamic/TensorFlow/TextClassification.cs Outdated
// ML.NET Pipeline

Action<IMDBSentiment, IntermediateFeatures> ResizeFeaturesAction = (i, j) =>
Action<IMDBSentiment, IntermediateFeatures> ResizeFeaturesAction = (i,
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be clearer if the (i, j) was on the same line

docs/samples/Microsoft.ML.Samples/Dynamic/TensorFlow/TextClassification.cs Outdated
.Append(mlContext.Transforms.CopyColumns("Prediction", "Prediction/Softmax"))
.Fit(dataView);
var engine = mlContext.Model.CreatePredictionEngine<IMDBSentiment, OutputScores>(model);
var model = mlContext.Transforms.Text.TokenizeIntoWords(
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to try and align the Append methods, as well as keep the transform strings together. Something like:

    var model =
    mlContext.Transforms.Text.TokenizeIntoWords(
        "TokenizedWords",
        "Sentiment_Text")
    .Append(mlContext.Transforms.Conversion.MapValue(
        "VariableLengthFeatures",
        lookupMap,
        lookupMap.Schema["Words"],
        lookupMap.Schema["Ids"],
        "TokenizedWords"))
    .Append(mlContext.Transforms.CustomMapping(
        ResizeFeaturesAction,
        "Resize"))
    .Append(tensorFlowModel.ScoreTensorFlowModel(
        "Prediction/Softmax",
        "Features"))
    .Append(mlContext.Transforms.CopyColumns(
        "Prediction",
        "Prediction/Softmax"))
    .Fit(dataView);

natke
natke approved these changes Jun 28, 2019
View reviewed changes
Copy link
Contributor

@natke natke left a comment

Choose a reason for hiding this comment

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

A couple of suggestions

codemzs
codemzs reviewed Jun 28, 2019
View reviewed changes
docs/samples/Microsoft.ML.Samples/Dynamic/TensorFlow/ImageClassification.cs
var image1 = Enumerable.Range(0, inputSize).Select(
x => (float)x / inputSize).ToArray();
var image2 = Enumerable.Range(0, inputSize).Select(
x => (float)(x + 10000) / inputSize).ToArray();
Copy link
Member

Choose a reason for hiding this comment

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

As discussed new line between any two lines that were broken into multiple lines.

@sayanshaw24 sayanshaw24 merged commit e0c4caa into dotnet:master Jun 29, 2019
Dmitry-A pushed a commit to Dmitry-A/machinelearning that referenced this pull request Jul 24, 2019
@sayanshaw24 @Dmitry-A
Reformatting TensorFlow and AnomalyDetection samples to width 85 (dot…
1f44c92 
…net#3922)

* Testing

* Reformatted TensorFlow samples to width 85

* Reformatted AnomalyDetection samples to width 85

* Fixed formatting errors

* Made a few edits to TensorFlow samples format

* Fixed syntax errors

* Spacing changes
@ghost ghost locked as resolved and limited conversation to collaborators Mar 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@codemzs codemzs codemzs left review comments

@artidoro artidoro Awaiting requested review from artidoro

+2 more reviewers

@natke natke natke approved these changes

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

5 participants

@sayanshaw24 @dnfclas @codemzs @natke @wschin